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] Support React 17 #23311

Merged
merged 3 commits into from Nov 2, 2020
Merged

[core] Support React 17 #23311

merged 3 commits into from Nov 2, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 29, 2020

Allows React 17 as a peer dependency in all packages.

Installing it in the main repo needs a bit more upstream/downstream work. Since we test the next release channel daily, it should be fairly safe. Most of the compat issues have already been resolved before 17 was released.

Closes #23306

Test plan

  1. fresh install with
    • codesandbox
    • npm (6 and 7)
    • yarn (1 and 2 v2 does not allow installs from urls i.e. can't be tested with csb deploys)

@eps1lon eps1lon added the new feature New feature or request label Oct 29, 2020
@@ -175,6 +175,7 @@
"**/@babel/types": "^7.10.2",
"**/cross-fetch": "^3.0.5",
"**/dot-prop": "^5.2.0",
"**/react-is": "^16.13.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

This avoids pulling in 16 and 17. But I haven't actually verified if yarn or yarn deduplicate are that aggressive so I'll catch up on that.

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 29, 2020

No bundle size changes

Generated by 🚫 dangerJS against e9a9cdb

"@types/react": "^16.8.6",
"react": "^16.8.0",
"react-dom": "^16.8.0"
"@types/react": "^16.8.6 || ^17.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

These aren't released yet. If npm and yarn don't complain on a fresh install we should get this in as well. @types/react@17 is already proposed in DefinitelyTyped/DefinitelyTyped#48971

@@ -52,7 +52,7 @@
"@material-ui/utils": "^5.0.0-alpha.14",
"clsx": "^1.0.4",
"prop-types": "^15.7.2",
"react-is": "^16.8.0"
"react-is": "^16.8.0 || ^17.0.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

This feels a bit awkward as a direct dependency but should "just work" in practice. More details why this is technically wrong can be found in facebook/react#20099.

@oliviertassinari
Copy link
Member

On a related note, did you have a look at the React 17 issue in #23215? I could only find one solution, but I'm not 💯% happy with it. Maybe you have some better ideas.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 29, 2020

On a related note, did you have a look at the React 17 issue in #23215?

I'll take a look. Though I would've preferred if the first advise is to not use React 17 yet instead of some timer workarounds. They likely break in more subtle ways.

@eps1lon eps1lon added the on hold There is a blocker, we need to wait label Oct 29, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Oct 29, 2020

I'll investigate #23215 first. Hopefully we can write a test that fails with 17 so we can rely on our nightly tests.

@oliviertassinari oliviertassinari removed the on hold There is a blocker, we need to wait label Oct 30, 2020
@eps1lon eps1lon merged commit c465fb6 into mui:next Nov 2, 2020
@eps1lon eps1lon deleted the feat/react-17-allow branch November 2, 2020 10:12
@oliviertassinari oliviertassinari changed the title [core] Allow React 17 [core] Support React 17 Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot install @material-ui/core with npm 7 and React 17
3 participants