-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[docs] Fix migration feedback #35232
Conversation
|
@@ -71,19 +71,36 @@ If you need to support IE 11, check out our [legacy bundle](/material-ui/guides/ | |||
|
|||
## Update React & TypeScript version | |||
|
|||
### Update React | |||
|
|||
The minimum supported version of React has been increased from v16.8.0 to v17.0.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off-topic
@mui/core I almost wonder if we shouldn't have moved in this direction: #25464. Most seems to keep the support of React 16.8.0
- https://github.com/ant-design/ant-design/blob/c2f75a2ba3054f67041810aa8d172ab5965b21b6/package.json#L309
- https://github.com/react-bootstrap/react-bootstrap/blob/bd016aa330b005073f79ec1439ad3fef5eb021b4/package.json#L150
- https://github.com/radix-ui/primitives/blob/f0a7495bcd00faa14164b2f80721149238c85d9d/packages/react/context-menu/package.json#L28
- https://github.com/adobe/react-spectrum/blob/810579b671791f1593108f62cdc1893de3a220e3/packages/react-aria/package.json#L63
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, if we support 17, I don't see why we shouldn't support 16.8. But we would need to probably add additional tests for it. I wonder if people still use it honestly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if people still use it
@mnajdova based on https://www.npmjs.com/package/react-dom?activeTab=versions I could cover an extra 7797292 weekly downloads of React DOM (downloads >=v16.8.0 && < v17.0.0). So with Material UI v5, we exclude 45% of the today's React DOM downloads.
But we would need to probably add additional tests for it.
We have tests for React 17 in
material-ui/.circleci/config.yml
Line 832 in 90d6a3b
react-17: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which have been broken for at least 3 months https://app.circleci.com/insights/github/mui/material-ui/workflows/react-17/overview?branch=master&reporting-window=last-90-days
Probably this means that it was a bad decision on my side to create this check as "non blocking" on the PR CI.
@mnajdova based on https://www.npmjs.com/package/react-dom?activeTab=versions I could cover an extra 7797292 weekly downloads of React DOM (downloads >=v16.8.0 && < v17.0.0). So with Material UI v5, we exclude 45% of the today's React DOM downloads.
I am not sure that this is that much black/white. If someone is not aiming to update React, it's likely that they are not going to update their UI library dependency either, so they may be depending on v4. But agree, it is a big percentage, so it may be worth checking what would take for us to support v16.8.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this means that it was a bad decision on my side to create this check as "non blocking" on the PR CI.
@mnajdova I think that a nice workaround could be to run this step randomly on PRs. This way, we get the feedback every now and then, without slowing down the CI too much.
so they may be depending on v4.
This could be why v4 is still this high in downloads, too many have to first upgrade React, but they might not because some other libraries are not compatible with React 17 yet. I have created a live app with Toolpad to better visualize v16.8 use, with live data: https://master--toolpad.mui.com/deploy/clcdobvof0008n99jpx86fede/pages/331kqzd
It seems to have been down during the holidays period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's bring this up with the team.
Co-authored-by: Olivier Tassinari <[email protected]>
Co-authored-by: Olivier Tassinari <[email protected]>
With npm: | ||
|
||
```sh | ||
npm update @material-ui/core@^4.11.2 react@^17.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix doc feedback about https://mui.com/material-ui/migration/migration-v4/
From @oliviertassinari
https://deploy-preview-35232--material-ui.netlify.app/material-ui/migration/migration-v4/#update-react-amp-typescript-version