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] Bump React to 19 #42824

Closed
wants to merge 42 commits into from
Closed

[core] Bump React to 19 #42824

wants to merge 42 commits into from

Conversation

aarongarciah
Copy link
Member

@aarongarciah aarongarciah commented Jul 2, 2024

Part of #42381

This PR serves as a sandbox to check how many issues we have left to solve to support React 19.

@aarongarciah aarongarciah changed the title Bump React to 19 [core] Bump React to 19 Jul 2, 2024
@aarongarciah aarongarciah added dependencies Update of dependencies React 19 support PRs required to support React 19 labels Jul 2, 2024
@aarongarciah aarongarciah self-assigned this Jul 2, 2024
@aarongarciah aarongarciah requested a review from DiegoAndai July 2, 2024 15:57
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jul 3, 2024
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jul 5, 2024
@DiegoAndai
Copy link
Member

@aarongarciah, we're working on the Emotion type fixes alongside the Emotion maintainers: emotion-js/emotion#3206

The next issue I'm facing is with the following packages:

  • styled-components used in the @mui/styled-engine-sc package, used for integrating with styled-components
  • next@13 used in the @mui/material-nextjs package.

Neither of these are up to date, and I don't think it's worth it for us to to try and update them like we did with Emotion:

  • The styled-components package is outdated (@types/react: 17) and not very active, there's a two year old React 18 issue with no response 😅: React 18 compatibility? styled-components/styled-components#3738
  • Version 13 of next is their previous major and I don't think updating would be a priority. Also, @mui/material-nextjs already supports next@14

So I think we should deprecate @mui/styled-engine-sc as well as the next@13 version of @mui/material-nextjs (keeping next@14 support). What do you think about this approach? The only problem I'm having is that I don't know how to freeze the React 18 for these deprecated packages as we cannot remove React 19 from the resolutions.

@aarongarciah
Copy link
Member Author

So I think we should deprecate @mui/styled-engine-sc as well as the next@13 version of @mui/material-nextjs (keeping next@14 support).

@DiegoAndai I think this makes sense.

The only problem I'm having is that I don't know how to freeze the React 18 for these deprecated packages as we cannot remove React 19 from the resolutions.

Can you expand a bit? I assume you're talking about the top level package.json resolutions. Can we add a resolutions key to @mui/styled-engine-sc to force React 18 types? Not sure how we'd solve the problem in @mui/material-nextjs only for nextjs@13.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 10, 2024
@mui-bot
Copy link

mui-bot commented Jul 10, 2024

Netlify deploy preview

ImageListItem: parsed: -1.34% 😍, gzip: -1.20% 😍
ToggleButtonGroup: parsed: -1.29% 😍, gzip: -1.19% 😍
MenuList: parsed: -1.30% 😍, gzip: -1.15% 😍
AvatarGroup: parsed: -1.22% 😍, gzip: -1.08% 😍
BottomNavigation: parsed: -1.34% 😍, gzip: -1.20% 😍
Accordion: parsed: -1.12% 😍, gzip: -0.98% 😍
@material-ui/core: parsed: -0.16% 😍, gzip: -0.10% 😍
TextField: parsed: -0.53% 😍, gzip: -0.32% 😍
@material-ui/lab: parsed: -0.28% 😍, gzip: -0.18% 😍
TabList: parsed: -0.79% 😍, gzip: -0.43% 😍
SpeedDial: parsed: -0.80% 😍, gzip: -0.43% 😍
Breadcrumbs: parsed: -0.85% 😍, gzip: -0.45% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 9bfbece

@DiegoAndai
Copy link
Member

DiegoAndai commented Jul 10, 2024

Hey @Janpot @michaldudak, I need your help with this. Could you check the branch and see if you can figure it out?

We have two packages which are not compatible with React 19 types:

  • styled-components, dependency of @mui/styled-engine-sc
  • next@13, dependency of @mui/material-nextjs

Important note: Focus only on the styled-components/@mui/styled-engine-sc and next@13/@mui/material-nextjs packages. If you run typescript on the root, you'll see many emotion type errors. Those are being worked on separately on emotion-js/emotion#3206.

Here are the errors I'm getting:

styled-components

(error reference)

../../node_modules/.pnpm/[email protected][email protected][email protected]_wix7kfhaaslmbrv34cmdpqmtmy/node_modules/styled-components/dist/types.d.ts(133,189): error TS2503: Cannot find namespace 'JSX'.

For this one, I would like to pin the @types/react version in @mui/styled-engine-sc, but I'm not able to achieve it. The things I tried are

  • Use resolutions in @mui/styled-engine-sc package.json. Doesn't work: resolutions` is for the root package only
  • Add specific package resolution as such:
     "@mui/styled-engine-sc>@types/react": "18.3.3",
     "@mui/styled-engine-sc>@types/react-dom": "18.3.0",
    but it doesn't seem to work either.

next@13

(error reference)

../../node_modules/.pnpm/[email protected]_@[email protected][email protected][email protected]_eyrhw4xacy4nwuqjvaiwg3fsum/node_modules/next/dist/shared/lib/utils.d.ts(158,59): error TS2503: Cannot find namespace 'JSX'.

For this one, I think we should deprecate the next@13 version of the package and turn off the typescript check, unless you have any suggestions on how to keep it working. I don't 😅.

@DiegoAndai
Copy link
Member

Also, I would expect this line to exclude the ../../node_modules/... from the typescript check, but it seems it doesn't 🤔

@JCQuintas
Copy link
Member

Going to sleep now, so can't help much, but got some points that might help you:

Also, I would expect this line to exclude the ../../node_modules/... from the typescript check, but it seems it doesn't 🤔

probably skipLibCheck: true on tsconfig can help here.

Use resolutions in @mui/styled-engine-sc package.json. Doesn't work: resolutions` is for the root package only

Resolutions is for yarn only, but we use pnpm, so you are probably looking at the wrong prop, you can use this instead

  "pnpm": {
    "overrides": {
      "@types/react": "18.3.3",
      "@types/react-dom": "18.3.0"
    }
  },

@aarongarciah
Copy link
Member Author

aarongarciah commented Jul 11, 2024

@JCQuintas resolutions gets merged with pnpm.overrides, and since we only use resolutions, it works. From the pnpm docs:

Functionally identical to pnpm.overrides, this field is intended to make it easier to migrate from Yarn.

resolutions and pnpm.overrides get merged before package resolution (with pnpm.overrides taking precedence), which can be useful when you're migrating from Yarn and need to tweak a few packages just for pnpm.

@JCQuintas
Copy link
Member

@JCQuintas resolutions gets merged with pnpm.overrides, and since we only use resolutions, it works. From the pnpm docs:

oops didn't know that. 🙃

@DiegoAndai
Copy link
Member

DiegoAndai commented Jul 11, 2024

probably skipLibCheck: true on tsconfig can help here.

Yes, skipLibCheck: true removes the warnings. I'm unsure if we want to use it (@mui/code-infra?). If there's a way for us to skip only these two stale library dependencies checks, that would be better than disabling it altogether. Feels like "throwing the baby out with the bath water".

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

probably skipLibCheck: true on tsconfig can help here.

Yes, skipLibCheck: true removes the warnings. I'm unsure if we want to use it (@mui/code-infra?). If there's a way for us to skip only these two stale library dependencies checks, that would be better than disabling it altogether. Feels like "throwing the baby out with the bath water".

As far as I know there is no other way around this right now, since you can't select which libs to skip in typescript.

Since we are already doing the "resolutions" change, that is a "trust me" moment. Sure the lib check might help catching so issues, but if the types are different, then you will definitely get an error with the only opt out I know being skipLibCheck: true.

ref:
microsoft/TypeScript#30511

@@ -267,7 +267,7 @@ export function useSlider(parameters: UseSliderParameters): UseSliderReturnValue

const [focusedThumbIndex, setFocusedThumbIndex] = React.useState(-1);

const sliderRef = React.useRef<HTMLSpanElement>();
const sliderRef = React.useRef<HTMLSpanElement>(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be?

Suggested change
const sliderRef = React.useRef<HTMLSpanElement>(undefined);
const sliderRef = React.useRef<HTMLSpanElement>(null);

It should be an immutable ref here. It looks like I have missed this one in #38762. We could make a PR just for it.

I dove a bit into this in mui/mui-x#11847.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, ok, it's changing: DefinitelyTyped/DefinitelyTyped#64896, so

Suggested change
const sliderRef = React.useRef<HTMLSpanElement>(undefined);
const sliderRef = React.useRef<HTMLSpanElement | null>(null);

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think | null is the correct approach, yeah #42881 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Right, then +1 to udpate all the codebase with this. We don't have to correlate it with React 19..it seems that it would work with all versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

For posterity, in React 19:

  • React.useRef requires an initial value. There's no need to do | null when the initial value is null (same for undefined). You can still do it if you want to be more explicit. It goes like this (pseudo-code):
    • const ref = React.useRef<T>(null) -> ref is T | null
    • const ref = React.useRef<T>(undefined) -> ref is T | undefined
    • const ref = React.useRef<T>(T) -> ref is T
  • React.MutableRefObject has been deprecated. React.RefObject is used for all cases now, and it's always mutable. Unlike React.Ref, we need to explicitly make the ref object nullable: React.RefObject<T | null>.

(more info in the React 19 types PR)

@Janpot
Copy link
Member

Janpot commented Jul 16, 2024

The things I tried are

I'll try to take a look at this over the week, but what I can already say about resolutions is that I don't think of it as a tool we can use to solve this problem. As far as I'm aware, as a package author, you can't enforce how your transitive dependencies are resolved by the consumers of your package. So we'd fix the problem in our repo, but I believe our users would still have it.

I think we shouldn't use resolutions/overrides, unless it's scoped to the docs workspace.

edit:
It looks like styled-components doesn't have a (peer)dependency on @types/react, so I guess that means there is nothing to override. I think I got it working with the following steps:

  • npm patch styled-components
  • open the package in code editor, edit the package.json to add to the peer dependencies:
    "@types/react": "*",
    "@types/react-dom": "*"
  • commit the patch
  • pnpm install --force

Not 100% clear to me yet what exactly is going on here. It's referencing react in its types without specifying react types as a peerDependency. Do we need to open PR with styled-components for this?

@DiegoAndai
Copy link
Member

DiegoAndai commented Jul 18, 2024

Thanks for taking a look @Janpot

I don't think of [resolutions] as a tool we can use to solve this problem

With this, do you mean that we shouldn't have @types/react and @types/react-dom in the resolutions? Or that we cannot use them for the styled-components issue in particular?


I tried the patch approach but I still get the same errors 😓

The issue here is that I'm not able to figure out how to make the pnpm workspace like this:

  • Use the React 19 types for most packages
  • Use React 18 types for mui-styled-engine-sc, or if not possible, do not test these types at all. We can deprecate this package or have a warning that it doesn't work with React 19, as I don't see them supporting React 19 anytime soon. This is the error we're trying to solve: reference
  • Use React 18 types for next@13 inside the mui-material-nextjs package, or if not possible, do not test these types at all. We can deprecate the next@13 version of mui-material-nextjsor have a warning that it doesn't work with React 19. This is the error we're trying to solve: reference

Do you see a way of making it work like that?

@oliviertassinari
Copy link
Member

deprecate @mui/styled-engine-sc as well as the next@13 version of @mui/material-nextjs

This feels a bit too bold. I could see a lot of people using styled-components and not care about types, as well as upgrading Material UI to v6 without upgrading React or Next.js. If we can ignore the types for those packages, I think it would be good enough.

@Janpot
Copy link
Member

Janpot commented Jul 19, 2024

I tried the patch approach but I still get the same errors

I'm taking a closer look today

With this, do you mean that we shouldn't have @types/react and @types/react-dom in the resolutions?

Correct, in fact we shouldn't have resolutions at all, resolutions are a bad idea, especially for library authors like us. Packages declare which dependency range they are compatible with, and the resolutions field instructs package managers to ignore that and supply another version that may or may not be compatible. Why would we expect this to just work? Moreover, this only happens inside of our repo, our users would get the transitive dependency installed the same way as if resolutions didn't exist. If pnpm dedupe doesn't dedupe it, something needs to be fixed at the dependency level, not faked with resolutions. It may work if you're building applications, but not when you're publishing libraries.

@emotion/... related type problems: I'm assuming them to be covered by emotion-js/emotion#3206. What I'm further changing in this PR:

  • Remove resolutions for @types/react. add a specific one that points @types/react inside of types-react-dom to types-react@19.... To be removed after @types/react@19 is released.
  • Add a resolution for @types/react in @types/hoist-non-react-statics. The latter has dependency of @types/react: * so this should be fine, to be removed after @types/react@19 is released.
  • patch styled-components to remove deprecated namespace. This patch needs to be proposed to styled-components repository and published to npm in a future version. All it needed is not accessing the global JSX namespace. I've seen them mix JSX. and React.JSX. so this shouldn't break any compatibility for them. update: relevant PR
  • @mui/material-nextjs (still investigating) This package has a peer dependency on next: 13 || 14. Both of which don't support React 19. Assuming that when users have any of these as a peer dependency, they're not using react 19 neither. The errors we see come from us adding v13 as a dev dependency, which I believe we do specifically for type checking. We can upgrade it to v15.0.0-canary.73 to make the errors go away, the end-users will be unaffected. Since this package basically depends on "whatever version the user has installed", there is no correct answer for this dev dependency. We're going to type check against react 19 types, so a a version of next that is compatible with React 19 types makes most sense to me.
  • docs (still investigating) I think I have most non emotion issues by resolving @types/react dependency of @types/react-swipable-views and @types/react-window

@DiegoAndai
Copy link
Member

If that works.

It's working locally 👍🏼 I'll need to check the CI builds "killed" signal issue, but it should also work in CI.

@DiegoAndai
Copy link
Member

Given the amount of dependency changes, medium sized containers receive the "killed" signal on CI. I temporarily changed the test_checkout to medium+ and it succeeded. With that, we can also run test_browser and test_types which already use medium+, and both are succeeding. I'll be using those for the moment.

When the time comes to merge this, we can change all test containers to medium+, and then change it back in a follow up PR. I don't know if there's an easier way to achieve this cc: @mui/code-infra, please let me know if there is.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 6, 2024
const { container } = render(<Data />);

expect(container.firstChild.textContent).to.equal('light:2'); // 2 because of double render within strict mode
expect(container.firstChild.textContent).to.equal('light');
expect(effectRunCount).to.equal(reactMajor >= 19 ? 2 : 3);
Copy link
Member

Choose a reason for hiding this comment

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

This test was recently added, that's why we didn't cover it before in this PR.

The previous approach didn't work with React 19, probably due to the changes to strict mode. The difference in events between 18 and 19 is the following:

 render
 render
 run effect
-run effect (this one doesn't run in React 19)
 render
 render
 run effect

And for both, the last effect run wasn't being registered in the rendered content. That's why with React 18 the result was light:2 even though the effect runs 3 times. For React 19, the effect is runs 2 times.

Copy link
Member

Choose a reason for hiding this comment

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

@DiegoAndai
Copy link
Member

DiegoAndai commented Dec 6, 2024

Moved to #44672 as Argos was not working due to this branch originating from the next branch

@DiegoAndai
Copy link
Member

Merged #44672

@DiegoAndai DiegoAndai closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies on hold There is a blocker, we need to wait React 19 support PRs required to support React 19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants