facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.61k stars 46.8k forks source link

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

Closed gurkerl83 closed 2 years ago

gurkerl83 commented 2 years ago

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 https://github.com/facebook/react/issues/24304#issuecomment-1094565891 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 https://github.com/DefinitelyTyped/DefinitelyTyped/commit/55dc209ceb6dbcd59c4c68cc8dfb77faadd9de12

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

Thx!

gurkerl83 commented 2 years ago

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

gaearon commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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.

eps1lon commented 2 years ago

All the type-related breaking changes are explained in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/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 https://github.com/DefinitelyTyped/DefinitelyTyped/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

It works very well, thanks

KutnerUri commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/testing-library/react-testing-library/issues/1000

gaearon commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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).

gaearon commented 2 years ago

I agree all of this sounds sad. However, in principle we need to be able to make breaking changes in major versions of types. I wonder what are the best practices around this in the TypeScript ecosystem, given that we are unlikely to be the first library that's a common peer and that made a breaking change in the type definition to fix an earlier mistake? One thing we could've done better on our side is to surface the list of TS breaking changes earlier. They were public (for over a year), but they were in a different repository, so I understand if people haven't seen them. (I saw... a hundred?... typing PRs linked in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/56210 by @eps1lon, and thank him for that heroic effort). We could have surfaced this work more (e.g. on our blog). Are there other things that we can learn from this? I appreciate your patience.

gtwebsite commented 2 years ago

Issue is developers placing * for deps. Thats just bad practice

Methuselah96 commented 2 years ago

Issue is developers placing * for deps. Thats just bad practice

@gtwebsite I don't believe that's actually the problem. Many of these types are in fact compatible with React 16, 17, and 18 types. The equivalent problem would still exist if they declared the dependency as @types/react@^16 || ^17 || ^18.

The issue is that package installation will always default to installing the latest version. I think the most helpful thing would be for DefinitelyTyped to make any usage of @types/react an optional peer dependency by default instead of a regular dependency and that way the users of @types package can control the @types/react version themselves.

wangly19 commented 2 years ago

resolutions is a temporary solution

"resolutions": {
    "@types/react": "17.0.2", // your version
    }
eps1lon commented 2 years ago

Incompatible versions of @types/react being installed is a recurring problem. We didn't release breaking changes in the types for quite some time now so I forgot this issue existed. This is an oversight on my part that we should've prepared for before releasing.

Declaring @types/react as an (optional) peer dependency is the correct solution. We've been doing this in MUI for a while now.

The reason you're seeing "@types/react": "*" in regular dependencies is most likely because it was copied over from packages published from the DefinitelyTyped repo.

We've had this discussion before (https://github.com/DefinitelyTyped/DefinitelyTyped/issues/33015 and others I believe) and are currently waiting for https://github.com/microsoft/DefinitelyTyped-tools/issues/433 (continued from https://github.com/microsoft/types-publisher/issues/431).

I'll write up an extensive use case for why we want optional peer dependencies to restart the discussion around dependencies vs peerDependencies and if we can avoid "*" ranges in the future.

workarounds

Yarn v1

npx yarn-deduplicate --packages @types/react for now. Any package requiring v17 explicitly should be asked to either bump it (if they support React 18) or you should find an alternative library anyway if you want to use React 18.

npm v8

You can use overrides like so:

{
  "overrides": {
    "@types/react": "^18.0.0"
  }
}

Note that you might still have to do some manual package-lock.json fixing with npm@8.5.0 (and below) until https://github.com/npm/cli/pull/4599 is released.

npm before v8

For npm before v8 I don't have a good solution. The only possible workaround is going nuclear and deleting package-lock.json and running npm install again

yarn v2 and above

Similar to Yarn v1: yarn dedupe '@types/react'. Any package requiring v17 explicitly should be asked to either bump it (if they support React 18) or you should find an alternative library anyway if you want to use React 18.

KutnerUri commented 2 years ago

this is not such a good solution, and it mostly works in monolith projects. wasn't React 18 supposed to work side by side with previous versions of React, allowing smoother gradual upgrades?

because right now it seems like we are forced to use either v18 or v17

gaearon commented 2 years ago

@KutnerUri

I understand the frustration, but this is the tradeoff you’re opting into by using TypeScript. We need to be able to fix bugs in definitions. Ultimately, if upgrading types is too much of a burden, it might be worth questioning whether TypeScript is really beneficial for your project.

Regarding gradual upgrades, 17 makes it possible by fixing the event system quirks. This doesn’t mean that we provide anything special for TypeScript definitions. You can file an issue in the TypeScript repo about this if you’d like to ask if there are good solutions for mixing versions. If you’re blocked by this, you can also upgrade React but keep all your types as 17, and then suppress typing errors everywhere that new methods are used.

In general though, we don’t think of the “mixed versions” upgrade strategy as very important for 18. It’s nice that it works, but in practice we’ve reduced the actual runtime breaking changes enough that in terms of effort, upgrading to 18 is comparable to our other major releases. So we’d suggest just upgrading the app at once.

Andarist commented 2 years ago

Also if this bothers anyone then you really don't have to upgrade right away - let the ecosystem figure the final way forward first and only attempt to upgrade later on. React 18 has been released just two weeks ago.

antonybudianto commented 2 years ago

For pnpm users:

"pnpm": {
  "overrides": {
    "@types/react": "17.0.37",
    "@types/react-dom": "17.0.11"
  }
}
sannajammeh commented 2 years ago

Here you go, this worked in my project.

// react.d.ts
import { FunctionComponent, ReactNode } from 'react';

declare module 'react' {
  declare namespace React {
    type FC<P = {}> = FunctionComponent<PropsWithChildren<P>>;
  }

  export = React;
  export as namespace React;
}
KutnerUri commented 2 years ago

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 🤔

tarlankarimli commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Still not fixed.

gaearon commented 2 years ago

Still not fixed.

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

I’ll close this issue to make it clearer.

jsarelas commented 2 years ago

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

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

tonnyESP commented 2 years ago

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

ghost commented 2 years ago

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

KutnerUri commented 2 years ago

(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 https://github.com/facebook/react/issues/24304#issuecomment-1097387935 is the way to go

KutnerUri commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

The workarounds in https://github.com/facebook/react/issues/24304#issuecomment-1094565891 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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/microsoft/DefinitelyTyped-tools/issues/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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/microsoft/DefinitelyTyped-tools/issues/433.

The short-term workarounds are described in https://github.com/facebook/react/issues/24304#issuecomment-1094565891.

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!