Skip to content

Conversation

@aarongarciah
Copy link
Contributor

@aarongarciah aarongarciah commented Jul 2, 2024

Breaking changes

Drop support for React < 16.8.0. Developers now need to type the ref differently since it isn't a class component anymore:

-const ref = React.useRef<Editor>(null);
+const ref = React.useRef<React.ComponentRef<typeof Editor>>(null);

In practice, few projects should be impacted by this change:

SCR-20240702-uqng

https://tools-public.mui.com/prod/pages/npmVersion?package=react-dom

@oliviertassinari: I believe it's time. React 19 continues to move away from class components, e.g. https://react.dev/blog/2024/04/25/react-19-upgrade-guide#removed-removing-legacy-context.

What changes

  • Refactors the component from class to function and stops using defaultProps.
    • Uses useImperativeHandle to expose the session object instead of a class property.
  • Updates the example to use a function component.
  • Updates the type declarations.

Future work:

  • Prepare the library for React 19 by using ref as a prop. This requires dependency updates and other changes that should be addressed in a future PR.

Context

While upgrading the material-ui repository to React 18.3.1, we were getting rid off defaultProps since React 18.3 started warning about defaultProps being deprecated on function components. Because Emotion copies the property into its function instance emotion-js/emotion#3184, a warning is triggered. Emotion should fix this, but we might as well modernize this component.

Test plan

All functionality should be intact. No expected changes. Run the example locally (yarn example) and compare it to https://react-simple-code-editor.github.io/react-simple-code-editor. Everything should work as expected.

I tested the material-ui docs locally and it works as expected.

cc @DiegoAndai

@aarongarciah aarongarciah changed the title Function component Convert to function component Jul 2, 2024
@aarongarciah aarongarciah changed the title Convert to function component Prepare for React 18.3 Jul 2, 2024
@aarongarciah aarongarciah marked this pull request as ready for review July 2, 2024 14:26
@aarongarciah aarongarciah changed the title Prepare for React 18.3 Support React 18.3 and modernize component Jul 2, 2024
@oliviertassinari
Copy link
Collaborator

oliviertassinari commented Jul 2, 2024

I will:

  • do a quick smoke test to see if I can spot any regressions
  • check behavior changes for existing users. See if we can release this as patch
  • get myself a bit more immerse into the logic, there are more problems that I would like to see solved for the docs experience, but until we fill the docs-infra role, I'm not sure anyone on the MUI's team would want to become an expert with this dependency.
  • release the changes

@oliviertassinari
Copy link
Collaborator

oliviertassinari commented Jul 2, 2024

We were getting rid off defaultProps since React 18.3 started warning about defaultProps being deprecated. Our last warning is caused by react-simple-code-editor using defaultProps.

@aarongarciah I guess you saw this on mui/material-ui#42047:

SCR-20240703-bkyf

This narrative for the motivation for the change looks false. I have updated the PR description. React is only deprecating defaultProps on function components.

Removed: propTypes and defaultProps for functions

https://react.dev/blog/2024/04/25/react-19-upgrade-guide#removed-proptypes-and-defaultprops

Here it's a class component. The error you observed looks like a bug of Emotion: emotion-js/emotion#3184. @LukasTy found the same problem on react-transition-group reactjs/react-transition-group#898 (comment). So I don't expect this PR to solve the problem for https://github.com/mui/material-ui. After we deploy this change, we will get a warning with react-transition-group (I assume React deduplicates this warning). No warnings on #126.

@oliviertassinari oliviertassinari changed the title Support React 18.3 and modernize component Migrate from class to hooks Jul 2, 2024
@oliviertassinari
Copy link
Collaborator

Rebased on #126.

@aarongarciah
Copy link
Contributor Author

aarongarciah commented Jul 2, 2024

@oliviertassinari yes, I was surprised to see this warning given it's a class component. But, I double-checked locally before opening this PR by removing the defaultProps from the class component, and the warning was gone.

I also checked locally with the changes in this PR (using file:/ to point to the local package), and the warning is gone. Even if Emotion is the source of the issue, removing defaultProps from react-simple-code-editor seems to solve the issue.

I haven't found any other warning like this in the material-ui docs. How can I reproduce the react-transition-group one?

@oliviertassinari
Copy link
Collaborator

oliviertassinari commented Jul 2, 2024

the warning was gone

@aarongarciah I agree, this PR will make the warning go away. My point is that it's not the root problem, so it shouldn't be why we merge this PR. This change should have the equivalent impact:

diff --git a/docs/src/modules/components/DemoEditor.tsx b/docs/src/modules/components/DemoEditor.tsx
index 25680a5d75..fb52af1f9e 100644
--- a/docs/src/modules/components/DemoEditor.tsx
+++ b/docs/src/modules/components/DemoEditor.tsx
@@ -63,6 +63,8 @@ const StyledSimpleCodeEditor = styled(SimpleCodeEditor)(({ theme }) => ({
   },
 }));

+delete StyledSimpleCodeEditor.defaultProps;
+
 interface DemoEditorProps extends React.HTMLAttributes<HTMLDivElement> {
   children: React.ReactNode;
   copyButtonProps: {};

emotion-js/emotion#3184 (comment) seems to be the fix at the root.


In any case, I'm merging, class components are the past. It's good to use "active" React APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants