pnpm (monorepo) project using wrong react types

danielleroux commented 9 months ago

Verify latest release

[X] I verified that the issue exists in the latest pnpm release

pnpm version

v8.8.0

Link to the code that reproduces this issue or a replay of the bug

https://github.com/siemens/ix/blob/migrate-to-pnpm/packages/react-test-app/src/preview-examples/aggrid.tsx

Reproduction steps

Checkout -> https://github.com/siemens/ix [branch: migrate-to-pnpm] or checkout extra repo https://github.com/danielleroux/pnpm-react-type-bug
Run pnpm run build --filter react-test-app

Describe the Bug

We started to migrate from yarn (turbo) to pnpm (turbo) but we are facing a problem with the types of one of our projects.

I dont have a clue why the build is reference the react types of version 17. (@types/react is only used in our documentation project by docusaurus)

Build output:

react-test-app:build: cache miss, executing e09cec3395d0cc71
react-test-app:build: 
react-test-app:build: > react-test-app@2.0.1 build /Users/daniel/dev/oss/ix-pnpm-migration/packages/react-test-app
react-test-app:build: > tsc && vite build
react-test-app:build: 
react-test-app:build: src/preview-examples/aggrid.tsx(62,8): error TS2786: 'AgGridReact' cannot be used as a JSX component.
react-test-app:build:   Its instance type 'AgGridReact<any>' is not a valid JSX element.
react-test-app:build:     Types of property 'refs' are incompatible.
react-test-app:build:       Type '{ [key: string]: import("/Users/daniel/dev/oss/ix-pnpm-migration/node_modules/.pnpm/@types+react@17.0.66/node_modules/@types/react/index").ReactInstance; }' is not assignable to type '{ [key: string]: React.ReactInstance; }'.
react-test-app:build:         'string' index signatures are incompatible.
react-test-app:build:           Type 'import("/Users/daniel/dev/oss/ix-pnpm-migration/node_modules/.pnpm/@types+react@17.0.66/node_modules/@types/react/index").ReactInstance' is not assignable to type 'React.ReactInstance'.
react-test-app:build:             Type 'Component<any, {}, any>' is not assignable to type 'ReactInstance'.
react-test-app:build:               Type 'import("/Users/daniel/dev/oss/ix-pnpm-migration/node_modules/.pnpm/@types+react@17.0.66/node_modules/@types/react/index").Component<any, {}, any>' is not assignable to type 'React.Component<any, {}, any>'.
react-test-app:build:                 The types returned by 'render()' are incompatible between these types.
react-test-app:build:                   Type 'import("/Users/daniel/dev/oss/ix-pnpm-migration/node_modules/.pnpm/@types+react@17.0.66/node_modules/@types/react/index").ReactNode' is not assignable to type 'React.ReactNode'.
react-test-app:build:                     Type '{}' is not assignable to type 'ReactNode'.
react-test-app:build:  ELIFECYCLE  Command failed with exit code 2.
react-test-app:build: ERROR: command finished with error: command (/Users/daniel/dev/oss/ix-pnpm-migration/packages/react-test-app) pnpm run build exited (1)

If i remove the workspace documentation which uses react@17 from the workspace yaml and perform a pnpm install the library builds correct.

Expected Behavior

Should use the @types/react@18 as declared in package.json of react-test-app (https://github.com/siemens/ix/blob/migrate-to-pnpm/packages/react-test-app/package.json#L26-L27)

Which Node.js version are you using?

18.18.0

Which operating systems have you used?

[X] macOS
[ ] Windows
[ ] Linux

If your OS is a Linux based, which one it is? (Include the version if relevant)

No response

danielleroux commented 9 months ago

I added a extra repository: https://github.com/danielleroux/pnpm-react-type-bug

Checkout the following file: https://github.com/danielleroux/pnpm-react-type-bug/blob/main/apps/react-example/src/App.tsx#L6

If you run:

pnpm run build --filter react-example

You will get the following output:

react-example:build: cache miss, executing 77acdb4c4f75772a
react-example:build: 
react-example:build: > react-example@0.0.0 build /Users/daniel/dev/oss/playground/pnpm-bug/apps/react-example
react-example:build: > tsc && vite build
react-example:build: 
react-example:build: src/App.tsx(6,8): error TS2786: 'AgGridReact' cannot be used as a JSX component.
react-example:build:   Its type 'typeof AgGridReact' is not a valid JSX element type.
react-example:build:     Type 'typeof AgGridReact' is not assignable to type 'new (props: any, deprecatedLegacyContext?: any) => Component<any, any, any>'.
react-example:build:       Construct signature return types 'AgGridReact<any>' and 'Component<any, any, any>' are incompatible.
react-example:build:         The types of 'refs' are incompatible between these types.
react-example:build:           Type '{ [key: string]: import("/Users/daniel/dev/oss/playground/pnpm-bug/node_modules/.pnpm/@types+react@17.0.67/node_modules/@types/react/index").ReactInstance; }' is not assignable to type '{ [key: string]: React.ReactInstance; }'.
react-example:build:             'string' index signatures are incompatible.
react-example:build:               Type 'import("/Users/daniel/dev/oss/playground/pnpm-bug/node_modules/.pnpm/@types+react@17.0.67/node_modules/@types/react/index").ReactInstance' is not assignable to type 'React.ReactInstance'.
react-example:build:                 Type 'Component<any, {}, any>' is not assignable to type 'ReactInstance'.
react-example:build:                   Type 'import("/Users/daniel/dev/oss/playground/pnpm-bug/node_modules/.pnpm/@types+react@17.0.67/node_modules/@types/react/index").Component<any, {}, any>' is not assignable to type 'React.Component<any, {}, any>'.
react-example:build:                     The types returned by 'render()' are incompatible between these types.
react-example:build:                       Type 'import("/Users/daniel/dev/oss/playground/pnpm-bug/node_modules/.pnpm/@types+react@17.0.67/node_modules/@types/react/index").ReactNode' is not assignable to type 'React.ReactNode'.
react-example:build:                         Type '{}' is not assignable to type 'ReactNode'.
react-example:build:  ELIFECYCLE  Command failed with exit code 2.
react-example:build: ERROR: command finished with error: command (/Users/daniel/dev/oss/playground/pnpm-bug/apps/react-example) pnpm run build exited (1)
react-example#build: command (/Users/daniel/dev/oss/playground/pnpm-bug/apps/react-example) pnpm run build exited (1)

If i remove the

    "@types/react": "^17.0.2",
    "@types/react-dom": "^17.0.2"

from the other project inside the repo (docusarus ~ my-website-ts) (https://github.com/danielleroux/pnpm-react-type-bug/blob/main/apps/my-website-ts/package.json#L30)

The build runs fine again, but of cause i lose all the typings for docusaurus.

hi-ogawa commented 8 months ago

I experienced similar issue when migrating multiple react apps into monorepo. One solution is suggested here:

https://github.com/pnpm/pnpm/issues/5176#issuecomment-1335507343

To fix this we had to add "react" to "paths" in tsconfig.json

So, in your case, adding this to tsconfig compilerOptions might help:

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

I think this is essentially forcing tsc's module resolution to not care with pnpm's base directory node_modules/.pnpm/... where all monorepo dependencies are linked to.

However, this approach might require many such tsc side of workaround depending on the number monorepo packages and also the number of such conflicting "types" dependencies.

In our case, actually we didn't know this idea at that time, so we ended up with using shared-workspace-lockfile = false option in .npmrc:

https://pnpm.io/npmrc#shared-workspace-lockfile

I think this ensures each monorepo package directory to have more traditional independent node_modules, so tsc will not be confused about react typing module resolution.

jfirebaugh commented 5 months ago

I've found that this is caused by third-party packages which include type declarations that use react types and have a phantom dependency on @types/react. These packages should list @types/react inpeerDependencies but it's fairly common for them to have it only as dev dependency, because with npm's hoisting behavior that phantom dependency typically won't cause issues. (And good luck trying to convince package maintainers that phantom dependencies are even a bug.)

RobertVillalba commented 3 months ago

My team just ran into this issue and it took forever to figure out this workaround.

michaelschufi commented 3 months ago

@hi-ogawa Thank you so much for this! I've been searching for ages. 🙏

The paths compiler option works, but since there's no way to know what packages are affected, we would like to go with a more robust workaround, like the non-shared lockfiles approach.

What are the disadvantages of non-shared lockfiles? The docs mention those advantages of a shared lockfile:

Advantages of this option:

every dependency is a singleton

faster installations in a monorepo

fewer changes in code reviews as they are all in one file

The one that's clear is the last one. For the others, it's rather difficult to know how much this affects installation times and other actions. What was your experience?

hi-ogawa commented 3 months ago

What are the disadvantages of non-shared lockfiles?

I would say perf aspect didn't feel like a problem (of course, it depends on the project though). More implicit disadvantage of shared-workspace-lockfile = false is that it's simply less battle-tested mode of pnpm, so I sometimes have to worry about whether some odd behaviors I'm seeing are specific to this mode.

You can search issues/pr to see some examples https://github.com/pnpm/pnpm/pulls?q=is%3Apr+shared-workspace-file+is%3Aclosed+. For example, sometime ago I fixed this minor bug https://github.com/pnpm/pnpm/pull/7252 which turns out to be specific to shared-workspace-lockfile = false.

Nonetheless, I'm quite happy with pnpm overall and I'm not sure if using alternative (npm, yarn) necessary helps this mixed typing issue.

michaelschufi commented 3 months ago

Thank you for sharing. I guess we'll try it with that flag and see how it goes. :-)

sagish commented 1 month ago

Also experiencing this when pnpm is not serving the correct React version when the repo has both React 17 and React 18 declared.

Would love this to have a stable solution w/o the workarounds 🙏

pnpm / pnpm