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

Bug: React 18 types broken since the type release a view hours ago #24304

Closed
gurkerl83 opened this issue Apr 8, 2022 · 47 comments
Closed

Bug: React 18 types broken since the type release a view hours ago #24304

gurkerl83 opened this issue Apr 8, 2022 · 47 comments

Comments

@gurkerl83
Copy link

gurkerl83 commented Apr 8, 2022

Edit: If you have issues but you did NOT upgrade to 18, or if you upgraded but get confusing errors from dependencies, the problem is likely that some library (incorrectly) specifies @types/react as a dependency with version * rather than an optional peer dependency. Find which library it is and file an issue for it. See #24304 (comment) for diagnostics and common workarounds.

A few hours ago, a major version of React types for Typescript was released.

I have tried to test this right away to see if there are any changes that require adaptation in my own projects.

Due to a very large number of users using React's type library (we use fundamental types like React.FC by the hundreds in our own projects), it's reasonable to question whether there's been a small mistake here.

Specifically, the following type declaration still exists in the 18 release.

type PropsWithChildren<P> = P & { children?: ReactNode | undefined };

However, this is no longer used in the library, at least not when looking at the diff
DefinitelyTyped/DefinitelyTyped@55dc209

A quick search on Github yields millions of hits that use the type alias React.FC to the actual type React.FunctionalComponent.

Before

interface FunctionComponent<P = {}> {
      (props: PropsWithChildren<P>, context?: any): ReactElement<any, any> | null;
      ...
}

Now

interface FunctionComponent<P = {}> {
      (props: P, context?: any): ReactElement<any, any> | null;
}

I think matching very, very many places in a single project doesn't seem to do justice to the change made; always the property children?: ReactNode to be redefined is definitely too much effort.

Positions at which PropsWithChildren were used before

  • FunctionComponent, as shown in the example
  • ForwardRefRenderFunction,
  • und das Funktions-Interface propsAreEqual von memo

Thx!

@gurkerl83 gurkerl83 added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 8, 2022
@gurkerl83
Copy link
Author

@eps1lon I saw you merged the following PR DefinitelyTyped/DefinitelyTyped#56210 . Can you please provide feedback on the matter with PropsWithChildren from above please?

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2022

Can we move this issue discussion to the DefinitelyTyped repository? That's where the React typings are maintained, and you'll probably see more people who are directly involved and can provide their thoughts on them.

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2022

If I understand correctly, the issue you're referring to is https://solverfox.dev/writing/no-implicit-children/. It's not strictly related to React 18 per se, but it's a breaking change the maintainers of React typings have meant to do for a few years. I don't want to speak for them — but there'll probably never be a particularly “good time” to do it, and as it stands the types were wrong. So at some point there is a need to bite the bullet. It looks like there is an automated conversion available so maybe this is something you can try: https://github.com/eps1lon/types-react-codemod#implicit-children.

@gurkerl83
Copy link
Author

@gaearon I will try the code-mod and provide feedback.

So at some point there is a need to bite the bullet.
Totally agree with this!

For others who may not be familiar with the definitely-typed repo, it would be great to get some orientation here.

@gaearon gaearon added Type: Discussion and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Apr 8, 2022
@eps1lon
Copy link
Collaborator

eps1lon commented Apr 8, 2022

All the type-related breaking changes are explained in DefinitelyTyped/DefinitelyTyped#56210. Specifically implicit children have a lengthy explainer https://solverfox.dev/writing/no-implicit-children/.

For migration of your own codebase you might want to take a look at https://github.com/eps1lon/types-react-codemod.

That doesn't help with library types though. For errors in node_modules I'm currently working on an approach that restores as much of the React 17 types behavior as possible (see DefinitelyTyped/DefinitelyTyped#59751). I can't guarantee a timely release at the moment.

I'm aware of the headaches this release causes and hope that you can understand why we made these changes. Some of these changes have been scheduled for years now and we already skipped the v17 release for that.

@ajereos-circadence
Copy link

ajereos-circadence commented Apr 8, 2022

quick and dirty, install npm-force-resolutions and add resolution for all of @types/react

on scripts,

"preinstall": "npx npm-force-resolutions"

on resolutions

"@types/react": "17.0.30"

@dstaver
Copy link

dstaver commented Apr 8, 2022

I totally understand that breaking changes are needed sometimes.

I only want to suggest that the next time you add a deprecation warning with a /**@deprecated*/ jsdoc tag in an intermediate release before releasing the breaking changes.

I would have stopped using this syntax a long time ago if my editor started warning me this was deprecated syntax.

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2022

I would have stopped using this syntax a long time ago if my editor started warning me this was deprecated syntax.

Can you clarify which syntax you're referring to?

@dstaver
Copy link

dstaver commented Apr 8, 2022

Specifically children being a prop on React.FC. I just checked, and in our codebase that type is used in 258 places. I haven't yet checked how many of them assume children exist. It's probably easy to fix, it was just an unexpected obstacle to testing out the new React version.

I also noticed a few dependencies breaking, React Helmet (recent version) and theme-ui (older version) among others. I already reverted the upgrade so I don't have more details unfortunately.

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2022

I see. I'm not sure /**@deprecated*/ would work there — wouldn't it flag the usage of children by passing them? Whereas the thing we'd want to flag is the other way around — it not being declared explicitly.

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 8, 2022

That might have worked: https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAKjgQwM5wEoFNkGN4BmUEIcA5FDvmQNwCwAUIzAJ5hZwAKxYqA6sBgALACJYwlXMhhYAJgGEhwADazKAOwA83CLwB8cALxce6AGRwA3ozi24AegQIbdxHAACs8ZOly4AWi5lHFQOL1xlZEo4AANcJVUNGLgsAA8wZWBcQWUWOAh1OGEOHV4itiwAOhc7BHsauHiVNSx1AH4ALkwqGErsPBgAOQgvegYAX0ZGYHUZKAI8EtMrBqbE1s7ugb6e4dHGSaYGVnYt-ABGAHYAMQBXdXxgAvkSSHVWmG1l40txg2N+vhKncHjAnuoXuACh8vrp+IJRN4sFIZAoEi0tKVUHo9GMpgxcAVUPAAJLgTLZGCKZoaLqAmBXEGPZ6vaGzIxwAAUljWGPGAEojAZ1LdlMoxoT1MS4ABRdIUwTU9bqOk9Rn3ZkQ1nvWaw-Qc7m8jQCoVwEVivEMTRkjJZRXojR6GTEzT2G0KqkO1q4xiaOW2ylKjFOrAu+z+j1Bx00IA. Though it is also potentially confusing since children aren't deprecated. It's their implicit declaration. But the deprecation message isn't included in the tooltip and instead users need to inspect @types/react to understand why children is marked as deprecated.

I like the idea and definitely worth trying next time. But it's not clear that this would've been a net positive.

It's also not like we didn't deprecate anything. The utility types that were removed were marked as deprecated for a long time.

@dstaver
Copy link

dstaver commented Apr 8, 2022

I agree that the typing for this seems tricky. The missing deprecation message is unfortunate and seems to be a weakness of Typescript. It happens when you destructure children in the arguments. If you use it in the function body it does show up:

const ImplicitChildren: React17FunctionComponent = (props) => {
    return props.children
};

The user experience of getting deprecation warning in advance of things changing is nice though - if possible to achieve. After a recent minor Mobx upgrade I started seeing these in the logs when running in development mode and when running tests:

[mobx-react-lite] observer(fn, { forwardRef: true }) is deprecated, use observer(React.forwardRef(fn))

It's a nice heads up to get, letting me know I should start updating the code in advance of thing actually breaking.

@caiorrsdeg
Copy link

quick and dirty, install npm-force-resolutions and add resolution for all of @types/react

on scripts,

"preinstall": "npx npm-force-resolutions"

on resolutions

"@types/react": "17.0.30"

if you use yarn, just use the resolutions, no need for npm-force-resolutions

@chlbri
Copy link

chlbri commented Apr 10, 2022

@types/react@18 break @types/styled-components DefinitelyTyped/DefinitelyTyped#59765

It works very well, thanks

@KutnerUri
Copy link

This breaks everything! 😭
I have a React 15 project that's happy with not upgrading to React 18 right away.
Because a LOT of packages depend on "@types/react": "*", version 18 is getting installed without anyone using it!

How long are you going to release this outdated @types/react which falls out of sync with the react package?
can you include built in types like other modern libraries?

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2022

Because a LOT of packages depend on "@types/react": "*", version 18 is getting installed without anyone using it!

Please file an issue in those packages. I’m not sure I understand why a library would need to include React types in its dependencies at all.

@Tobbe
Copy link

Tobbe commented Apr 10, 2022

I’m not sure I understand why a library would need to include React types in its dependencies at all.

@testing-library/react does this. Here's the Issue (links to PR) describing why they had to do that testing-library/react-testing-library#1000

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2022

Interesting. This does seem like a potential pitfall when new versions come out. Shouldn't it be a peerDependency? Maybe @eps1lon knows more about how this is supposed to work.

@markerikson
Copy link
Contributor

@gaearon This is sorta necessary for libraries that ship TS types. TS compiles app source, which refers to LibA types, which point to React types. So, either you explicitly say "LibA depends on @types/react" to ensure that they get installed, or you peerDep them.

I was actually just debating this with @Andarist for React-Redux v8, and what we settled on was an optional peer dep:

  "peerDependencies": {
    "react": "^18.0.0"
  },
  "peerDependenciesMeta": {
    "@types/react": {
      "optional": true
    },

so we're assuming that the user will already have those types installed if they plan on using React-Redux with TS. But, given that this is a relatively newer thing for package.json (?), it's very understandable that a bunch of libs would have explicitly listed that as a standard dep in order to make sure it gets installed into the end user's project.

@Methuselah96
Copy link

Methuselah96 commented Apr 11, 2022

Agreed that an optional peer dependency is the best option at the moment for library maintainers. That way users are more in control of which version is installed.

However @types packages hosted by DefinitelyTyped currently put @types/react in dependencies, so users will somehow have to make sure they have a single copy of the right @types/react version (either through Yarn resolutions, npm-force-resolutions, npm 8 overrides, or manually unifying the @types/react entry in their lock file).

@KutnerUri
Copy link

the odd thing about the types is that they are global somehow. I have a React15 project with a react 17 sub project in a folder inside it. The react18 types from the sub folder somehow cause errors in the main project 🤔

htbkoo added a commit to htbkoo/personal-portfolio that referenced this issue Apr 13, 2022
…owngrade `@types/react` and `@types/react-dom` back to `v17.x.x` because of the breaking changes in version 18 that could not be fixed until either material-ui is upgraded to v5 or this PR (DefinitelyTyped/DefinitelyTyped#59751) is merged

Note this codemod (https://github.com/eps1lon/types-react-codemod) would not help because the errors are caused by library (see this facebook/react#24304 (comment) for the details)

References:
1. https://stackoverflow.com/questions/71810438/compilation-issue-with-react-typescript-and-material-ui-4
2. DefinitelyTyped/DefinitelyTyped#56210 (comment)
3. facebook/react#24304 (comment)
4. https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/
5. facebook/react#24304 (comment)
@tarlankarimli
Copy link

quick and dirty, install npm-force-resolutions and add resolution for all of @types/react

on scripts,

"preinstall": "npx npm-force-resolutions"

on resolutions

"@types/react": "17.0.30"

I got more errors after applying this method

@ericgruss
Copy link

npm 6

Edit the package-lock.json file to remove the "*" in the types to the version you are using then use "npm ci" which will use your package-lock file to do the install. If you add other dependencies you may just need to update the lock file again, but you can have a stable build.

@wasurocks
Copy link

Still not fixed.

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2022

Still not fixed.

There’s nothing to be fixed here. Please read the discussion.

I’ll close this issue to make it clearer.

@jsarelas
Copy link

jsarelas commented Apr 19, 2022

I was able to resolve this issue with a package.json change:

"optionalDependencies": {
"@types/react": "^17.0.44"
},

@tonnyESP
Copy link

We had the same problem today.
In our stack we are using NextJS with React 17. One dependency in the project is using "@types/react": "*" so it was resolving to v18.

I have forced to use version 17 of the package with:

npm i @types/react@17.0.39 --D --force

and it stopped resolving to version 18, so it is now working again.

Maybe it will help you

@waynebloss
Copy link

@xxxxue This is what yarn resolutions does for you - https://classic.yarnpkg.com/lang/en/docs/selective-version-resolutions/

@KutnerUri
Copy link

(just for reference)
I think I found the main incompatibility:

in a typical component you will have a mix of "native" react elements and custom react components, like:

import React from 'react';
import type { ReactNode } from 'react';

function Comp({children}: {children?: ReactNode}) {
  return <div>
    {children}
  </div>
}

The problem actually comes because the type of <div/> is global:

../../bit-bin/scopes/ui-foundation/use-box/tab-content/tab-content.tsx:13:38 - error TS2322: Type 'import("/Users/kutner/projects/bit-bin/node_modules/@types/react/index").ReactNode' is not assignable to type 'React.ReactNode'.

13       <div>{children}</div>
              ~~~~~~~~~~

  node_modules/.pnpm/registry.npmjs.org+@types+react@18.0.7/node_modules/@types/react/index.d.ts:1375:9
    1375         children?: ReactNode | undefined;
                 ~~~~~~~~
    The expected type comes from property 'children' which is declared here on type 'DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>'

So while Comp is using ReactNode directly from import { ReactNode } from 'react', the children type of <div> is global, and comes from whichever @types/react was loaded last.

This almost guarantees that any project using @types/react versions 17 and 18 will break. In my case it's because I have multiple projects linked together.
Maybe #24304 (comment) is the way to go

@KutnerUri
Copy link

KutnerUri commented Apr 26, 2022

ok I think I found the breaking change:

// in @types/react v17:
type ReactFragment = {} | ReactNodeArray;

// in @types/react v18:
type ReactFragment = Iterable<ReactNode>;

which is used in

DOMAttributes {
  children?: ReactNode;
}

declare global {
  namespace JSX {
    interface IntrinsicElements {
      div: DetailedHTMLFactory<HTMLAttributes<HTMLDivElement>, HTMLDivElement>;
      ...
    }
  } 
}

I'm trying to see if I create a compatibility layer, like react.compat.d.ts, or force typescript to use @types/react 18 even thought it's spanning two projects.

I wish JSX was not global and so would not effect the project still using @types/react v17 😭

@KutnerUri
Copy link

KutnerUri commented Apr 27, 2022

I managed a workaround using typescript's paths option:

{
  "compilerOptions": {
    "paths": {
      "react": [ "./node_modules/@types/react" ]
    }
  }
}

This tells Typescript to always resolve "react" from this location. It works better than resolutions because it forces typescript to use this single instance, solving my complicated use-case with two linked workspaces.

Looking deeper into the breaking changes, and fixing errors as I upgrade to v18, I see they are really important.
For example, some package maintainers used ReactNode instead of ComponentType, which is wrong and somehow works in v17.

Since there are almost no breaking changes in v18, it seems the only way out is forward. It will be tricky not to break my consumers.

I do wish that @types/react did not declare global types, which made a lot of this mess in the first place, though I can't think of a better solution.

@ctjlewis
Copy link

ctjlewis commented Apr 27, 2022

Candidate for worst design choice of the year. Nothing better than being locked into this ecosystem.

Now every FC definition is broken, and you'll have to manually specify props already inherent to a component like children. It's actually such a mess that even trying to freeze your versions to v17 will result in a type error over this because @types/react-dom@17.0.2 depends on @types/react@* which pulls v18 types (see multiple SO issues).

What possible benefit is received in exchange for all of this?

@gaearon
Copy link
Collaborator

gaearon commented Apr 27, 2022

because @types/react-dom@17.0.2 depends on @types/react@* which pulls v18 types

The workarounds in #24304 (comment) address this. It sucks that there need to be workarounds, but ultimately we need to be able to make breaking changes to types in major versions. If you dislike the experience, the TypeScript and/or DefinitelyTyped tooling repositories are the right place to complain, since the practice of using * for versions is not encouraged by npm and comes directly from DefinitelyTyped. This practice is wrong and leads to consequences like this.

What possible benefit is received in exchange for all of this?

The major benefit is that the compiler no longer lets objects through (which crash at runtime). The issue is described in https://fettblog.eu/react-types-for-children-are-broken/.

@gaearon
Copy link
Collaborator

gaearon commented Apr 27, 2022

I want to reiterate that using TypeScript is a choice. You don't have to use React with TypeScript. If you choose to use React with TypeScript, the problems with the TypeScript ecosystem will affect you. The pervasiveness of the * dependency range is a problem with the TypeScript ecosystem, and you should bring it up with the TypeScript project. We don't have the leverage to fix this.

@DanielRosenwasser
Copy link
Contributor

DanielRosenwasser commented Apr 27, 2022

Hey all, I want to provide some context from the TypeScript/DT side.

First, the current state of the world doesn't have an immediate fix that makes everything good and easy again. I don't think we can feasibly do anything without breaking other existing users. I'm sorry that things have ended up rocky here.

If you're dealing with duplicate installations of @types/react, the best guidance I can provide now is to either create a new lockfile, or use the overriding or deduplicating logic of your package manager, as described here here.

Switching existing packages to move @types/react from dependencies to peerDependencies would break a different group of users in a different way (though admittedly, those who would be broken have arguably questionable package.jsons). What's more, the behavior of package managers and their versions varies a decent amount, regardless of newer fields like peerDependenciesMeta. So changing an existing package over to peerDependencies now would be an arguably another break.
So what can we do?

It may be worth revisiting peerDependencies long-term and for newer packages. That discussion is taking place over at microsoft/DefinitelyTyped-tools#433. That will take some figuring out, and it'll take time for the team to (re)build context on each of the problems that that would introduce (including UX issues, automatic type acquisition issues, etc.). I can't give a timeline on that investigation right now, but it seems like a reasonable long-term investment to unlock libraries from using it. But it's not clear at this point in time whether it's something we'd want every React consumer to use in place of regular dependencies.

Otherwise this thread is fairly long, and it doesn't seem to be getting any more productive. It may be worth locking the thread so users can easily see the workarounds I've mentioned above.

@enoh-barbu
Copy link

I want to reiterate that using TypeScript is a choice. You don't have to use React with TypeScript. If you choose to use React with TypeScript, the problems with the TypeScript ecosystem will affect you. The pervasiveness of the * dependency range is a problem with the TypeScript ecosystem, and you should bring it up with the TypeScript project. We don't have the leverage to fix this.

well, we should then complain to "@types/react" ? :)

@gaearon
Copy link
Collaborator

gaearon commented Apr 27, 2022

well, we should then complain to "@types/react" ? :)

No because the problem isn’t with the React typings. It’s with the way typing dependencies are expressed in the TS ecosystem. You could argue that the problem is with every package that specifies version * as a dependency. But as noted by @DanielRosenwasser, there is no immediate universal fix on their side either at this moment.

The longer-term solution to this is being discussed in microsoft/DefinitelyTyped-tools#433.

The short-term workarounds are described in #24304 (comment).

This thread is starting to go in circles. I’m afraid there isn’t anything actionable on the React side here so I’m going to lock this thread as resolved. If you believe there’s something actionable for React, please file a new issue and let us know how we can help.

Thank you!

@facebook facebook locked as resolved and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests