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

[core] Add React 18 to peer dependencies #4332

Merged
merged 3 commits into from
Apr 11, 2022

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Apr 2, 2022

CodeSandbox: https://codesandbox.io/s/datagridprodemo-material-demo-forked-qvr2t0?file=/demo.tsx

In the comments, I explain the changes I had to make. I updated the peer deps of the date picker components but I don't know if they are compatible, are they? We didn't really test it in React 18 so one way to review this PR could be to open the example above and check if all features still work as expected. The tests are passing in React 18. #4155

@m4theushw m4theushw added the core Infrastructure work going on behind the scenes label Apr 2, 2022
@mui-bot
Copy link

mui-bot commented Apr 2, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 348.1 409.7 350.5 367.14 24.234
Sort 100k rows ms 461.4 893.8 721.9 690.88 140.71
Select 100k rows ms 104.2 186.3 124 136.96 28.779
Deselect 100k rows ms 99.3 288.1 193.5 178.22 62.821

Generated by 🚫 dangerJS against 462bd07

updateRenderContext(nextRenderContext);
// Prevents batching render context changes
ReactDOM.flushSync(() => {
updateRenderContext(nextRenderContext);
Copy link
Member Author

@m4theushw m4theushw Apr 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the following glitch:

msedge_Z3qsBKGClx

From what I found, React 18 doesn't commit some renders if they take too much time. But, no matter the case, we update the render zone position so the position ends not matching the context rendered. Flushing the tree ensures that no render will be skipped. facebook/react#18402 (comment)

Updating the position inside React.useLayoutEffect also didn't work quite well. Here's the result:

msedge_ARUYFaWVCh

I would like to see how other virtualization libraries will solve this problem. Until there we have to use this workaround. If you have suggestions to improve this please comment.

// To prevent flickering, the inner position can only be updated after the new context has
// been rendered. ReactDOM.flushSync ensures that the state changes will happen before
// updating the position.
ReactDOM.flushSync(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes

msedge_c95qQZ4C70

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could not find any issues, I guess we will have better feedbacks once the community start using it

@cherniavskii
Copy link
Member

Should we start using React 18 in our docs? This may help to discover issues sooner. What do you think?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 11, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 11, 2022
@m4theushw m4theushw merged commit df9f7dd into mui:master Apr 11, 2022
@m4theushw m4theushw deleted the add-react-18-to-peer-deps branch April 11, 2022 16:17
@m4theushw
Copy link
Member Author

Should we start using React 18 in our docs? This may help to discover issues sooner. What do you think?

@cherniavskii I'm waiting for the core to update first. Keeping two versions of React may not work correctly. Once the core updates we can merge #4155.

@fourteenmeister fourteenmeister mentioned this pull request Apr 14, 2022
6 tasks
@jeveloper
Copy link

Thank you and i'm glad to see these amazing changes.
However, wouldn't the main lib mui/material be in conflict as it stricty supports only up to react 17?
I don't assume the main lib is getting overhauled this quickly to match depdencies.

Thanks

@m4theushw
Copy link
Member Author

However, wouldn't the main lib mui/material be in conflict as it stricty supports only up to react 17?

@jeveloper No, the latest version of @mui/material is already updated to support React 18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants