Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade class components to functional components #9712

Closed
72 of 99 tasks
Tracked by #9864 ...
joshblack opened this issue Sep 21, 2021 · 12 comments
Closed
72 of 99 tasks
Tracked by #9864 ...

Upgrade class components to functional components #9712

joshblack opened this issue Sep 21, 2021 · 12 comments
Labels
planning: umbrella Umbrella issues, surfaced in Projects views

Comments

@joshblack
Copy link
Contributor

joshblack commented Sep 21, 2021

Why are we doing this?

According to the React team, switching a component from a class component to a functional component is technically a breaking change. This is because class components accept a ref that points to the class instance by default. There is no way to opt-out of this behavior. As a result, if we switched a component to a functional component it would no longer have a ref, and existing code that uses a ref would break. Similarly, using React.forwardRef would produce a component where the ref would no longer point to a class instance and code could similarly break.

We have moved some components over to the new format despite these situations. The rationale at the time was that we weren't intending people to use these parts of a component (specifically class instance methods) and if teams were trying to use the ref to find a node in the document their code would work if we used React.forwardRef.

With v11, it seemed like a great opportunity to convert our remaining class components to the new functional component style with hooks.

Effort size legend
❇️ - sm
✴️ - md
🆘 - lg

Tasks

We would like to convert the following components to functional components that use hooks where appropriate:

@joshblack joshblack changed the title Upgrade class components to functional components @jnm2377 @sstrubberg Upgrade class components to functional components Sep 21, 2021
@joshblack
Copy link
Contributor Author

joshblack commented Sep 21, 2021

Recipes

// TEMPLATE
function ComponentName(props, ref) {}

const ComponentName = React.forwardRef(
  function ComponentName(props, ref) {}
);

ComponentName.propTypes = {};
  1. Start off with the template of the component
  2. Grab the render() method and paste it into your new component
  3. Grab the propTypes and paste them into your new component
  4. Move over any default props, where appropriate
  5. Initialize state in local variables using React.useState from the constructor
  6. Move over any event handlers into the body of the component
  7. Fix everything that breaks (change this.* to local variables)

Special cases

Situations where we would not want to migrate

  • The capability of the component is not available in hooks
    • componentDidCatch has no hooks equivalent
  • The cost of migrating is too high and we're changing the component anyways

How to use in monorepo

  • Create a next folder in the component folder that you're migrating
    • src/components/Notification/next/*
  • Create a component toggle in the entrypoint of the component folder
import * as FeatureFlags from '@carbon/feature-flags';
import { OverflowMenu as OverflowMenuNext } from './next/OverflowMenu';
import OverflowMenuClassic from './OverflowMenu';

export const OverflowMenu = FeatureFlags.enabled('enable-v11-release')
  ? OverflowMenuNext
  : OverflowMenuClassic;

@sstrubberg
Copy link
Member

example for how to handle getDerivedStateFromProps

function TestComponent({ example }) {
  const [value, setValue] = useState(example);

  useEffect(() => {
    setValue(example);
  }, [example]);
}

becomes

function TestComponent({ example }) {
  const [value, setValue] = useState(example);
  const [prevExample, setPrevExample] = useState(example);

  if (example !== prevExample) {
    setValue(example);
    setPrevExample(example);
  }
}

@tay1orjones
Copy link
Member

Another thing to note on default vs named exports - the src/index.js is typically expecting a default export. So if you're going to do a conditional export, it might be easiest to ensure it's default exports all the way through the export chain.

For instance:

// MyFunComponent/next/MyFunComponent.js

export default function MyFunComponent({...props}) {
return (
    <div>{...props}</div>
  );
}

// MyFunComponent/index.js
import { default as MyFunComponentNext } from './next/MyFunComponent';
import { default as MyFunComponentClassic } from './MyFunComponent';

export default MyFunComponent = FeatureFlags.enabled('enable-v11-release') ? MyFunComponentNext : MyFunComponentClassic;

@jnm2377
Copy link
Contributor

jnm2377 commented Oct 6, 2021

We also need to add a new test file in the /next folder for each component and update the test to pull in the v11 component.

@joshblack
Copy link
Contributor Author

joshblack commented Oct 25, 2021

Checklist

  • Create the new folders and files, if they don't already exist
    • Create a src/components/{component}/next/ folder
    • Create a src/components/{component/next/{component}.js file
    • Create a src/components/{component/next/__tests__ folder and copy over tests
  • Add a barebones component to the component file created above
  • Copy over prop types
  • Copy over props
  • Refactor anything lifecycle related
  • Refactor anything using this (like event handlers or refs)
  • Update the src/componenots/{component}/index.js to use conditional exports
  • Check tests to see if they fail, fix ones that fail or refactor into a new format if applicable
  • Check the development environments to make sure nothing breaks
    • Classic storybook (packages/react)
    • v11 storybook (packages/carbon-react)
  • Look for general refactor / changes
    • classnames -> cx
    • other -> rest
    • Remove anything that says "remove in v11"
  • Add an overview of these changes to the v11-carbon-components-react.md file

Questions

  • What should we do with stories? Do we duplicate for next? Do we add it to the next folder?
  • Can we figure out ways to run only enzyme or only RTL?

@motou
Copy link
Contributor

motou commented Oct 26, 2021

I can take over the task for the component:

  • src/components/TileGroup/TileGroup.js
    ✴️ TileGroup

@jnm2377
Copy link
Contributor

jnm2377 commented Oct 26, 2021

Awesome! Thanks @motou we appreciate the contribution! I'll assign it to you. If you have any questions about the implementation, feel free to look at the other PRs we've opened so far, or ask questions here. 😄

@motou
Copy link
Contributor

motou commented Oct 27, 2021

@jnm2377 would you please approve the running workflows for #9955? Thanks!

@motou
Copy link
Contributor

motou commented Oct 27, 2021

@joshblack

Checklist

  • Add an overview of these changes to the v11-carbon-components-react.md file

the right file path is docs/migration/11.x-carbon-components-react.md

@motou
Copy link
Contributor

motou commented Oct 27, 2021

Moreover, I can work on the following components:

  • src/components/ProgressIndicator/ProgressIndicator.js
    ✴️ ProgressIndicator

  • src/components/OverflowMenuItem/OverflowMenuItem.js
    ✴️ OverflowMenuItem

  • src/components/InlineCheckbox/InlineCheckbox.js
    ✴️ InlineCheckbox

@tay1orjones tay1orjones changed the title Upgrade class components to functional components [Umbrella] Upgrade class components to functional components Nov 1, 2021
@tay1orjones tay1orjones mentioned this issue Nov 2, 2021
21 tasks
@tay1orjones tay1orjones moved this to 🏗 In Progress in Design System Dec 10, 2021
@tay1orjones tay1orjones added the planning: umbrella Umbrella issues, surfaced in Projects views label Dec 10, 2021
@tay1orjones tay1orjones changed the title [Umbrella] Upgrade class components to functional components Upgrade class components to functional components Dec 10, 2021
@tay1orjones tay1orjones added this to the v11 Release Candidate 0 milestone Dec 13, 2021
@tay1orjones
Copy link
Member

Looks like this can be marked as complete based on status of #10281 👍

Repository owner moved this from 🏗 In Progress to ✅ Done in Design System Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
planning: umbrella Umbrella issues, surfaced in Projects views
Projects
Archived in project
Development

No branches or pull requests

5 participants