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

Move @types/react-reconciler to dependencies #2123

Conversation

Methuselah96
Copy link
Contributor

The built declaration files depend on @types/react-reconciler. Here are the build errors without it:

.yarn/__virtual__/@react-three-fiber-virtual-af98807581/0/cache/@react-three-fiber-npm-7.0.26-699ff8d26f-72548c6d2a.zip/node_modules/@react-three/fiber/dist/declarations/src/core/renderer.d.ts:2:24 - error TS7016: Could not find a declaration file for module 'react-reconciler'. 'C:/Users/nbier/Documents/stash/test-react-three-fiber/.yarn/__virtual__/react-reconciler-virtual-c1be2dd0c5/0/cache/react-reconciler-npm-0.26.2-284c00acc7-2ebceace56.zip/node_modules/react-reconciler/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/react-reconciler` if it exists or add a new declaration (.d.ts) file containing `declare module 'react-reconciler';`

2 import Reconciler from 'react-reconciler';
                         ~~~~~~~~~~~~~~~~~~

.yarn/__virtual__/@react-three-fiber-virtual-af98807581/0/cache/@react-three-fiber-npm-7.0.26-699ff8d26f-72548c6d2a.zip/node_modules/@react-three/fiber/dist/declarations/src/web/index.d.ts:1:23 - error TS2688: Cannot find type definition file for 'react-reconciler'.

1 /// <reference types="react-reconciler" />
                        ~~~~~~~~~~~~~~~~

.yarn/__virtual__/@react-three-fiber-virtual-af98807581/0/cache/@react-three-fiber-npm-7.0.26-699ff8d26f-72548c6d2a.zip/node_modules/@react-three/fiber/dist/declarations/src/web/index.d.ts:14:34 - error TS7016: Could not find a declaration file for module 'react-reconciler'. 'C:/Users/nbier/Documents/stash/test-react-three-fiber/.yarn/__virtual__/react-reconciler-virtual-c1be2dd0c5/0/cache/react-reconciler-npm-0.26.2-284c00acc7-2ebceace56.zip/node_modules/react-reconciler/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/react-reconciler` if it exists or add a new declaration (.d.ts) file containing `declare module 'react-reconciler';`

14 declare const reconciler: import("react-reconciler").Reconciler<unknown, unknown, unknown, unknown, unknown>, applyProps: (instance: import("../core/renderer").Instance, data: import("../core/renderer").InstanceProps | import("../core/renderer").DiffSet) => import("../core/renderer").Instance;
                                    ~~~~~~~~~~~~~~~~~~

.yarn/__virtual__/@react-three-fiber-virtual-af98807581/0/cache/@react-three-fiber-npm-7.0.26-699ff8d26f-72548c6d2a.zip/node_modules/@react-three/fiber/dist/declarations/src/web/index.d.ts:33:44 - error TS7016: Could not find a declaration file for module 'react-reconciler'. 'C:/Users/nbier/Documents/stash/test-react-three-fiber/.yarn/__virtual__/react-reconciler-virtual-c1be2dd0c5/0/cache/react-reconciler-npm-0.26.2-284c00acc7-2ebceace56.zip/node_modules/react-reconciler/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/react-reconciler` if it exists or add a new declaration (.d.ts) file containing `declare module 'react-reconciler';`

33 declare const act: (callback: () => import("react-reconciler").Thenable<unknown>) => import("react-reconciler").Thenable<void>;
                                              ~~~~~~~~~~~~~~~~~~

.yarn/__virtual__/@react-three-fiber-virtual-af98807581/0/cache/@react-three-fiber-npm-7.0.26-699ff8d26f-72548c6d2a.zip/node_modules/@react-three/fiber/dist/declarations/src/web/index.d.ts:33:93 - error TS7016: Could not find a declaration file for module 'react-reconciler'. 'C:/Users/nbier/Documents/stash/test-react-three-fiber/.yarn/__virtual__/react-reconciler-virtual-c1be2dd0c5/0/cache/react-reconciler-npm-0.26.2-284c00acc7-2ebceace56.zip/node_modules/react-reconciler/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/react-reconciler` if it exists or add a new declaration (.d.ts) file containing `declare module 'react-reconciler';`

33 declare const act: (callback: () => import("react-reconciler").Thenable<unknown>) => import("react-reconciler").Thenable<void>;
                                                                                               ~~~~~~~~~~~~~~~~~~


Found 5 errors.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9263a40:

Sandbox Source
pmndrs/react-three-fiber Configuration

@joshuaellis
Copy link
Member

This feels strange. I've used r3f for ages with TS without facing this build error. Are you using a special build set up?

Also, it was my understanding that types should always be a dev dependency?

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Mar 13, 2022

That likely means you have skipLibCheck set to true. That happens a lot in practice because tsc --init defaults skipLibCheck to true and fork-ts-checker-webpack-plugin by default overrides it to be true even if it's set to false in your tsconfig.json.

I created a quick repro to show the error with a very simple CRA app. If you take a look at the last CI build you can see it failing with these errors.

As for whether types packages go in dependencies or devDependencies, if the libraries declaration files depend on the types from a certain package then the types package should go in dependencies (1) (2).

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

I have no issues with this change then.

@drcmda
Copy link
Member

drcmda commented Mar 16, 2022

@Methuselah96 could you re-target to v8? or both? we're awaiting the r18 release and move forward with the v8 branch. this would cause another merge conflict.

@Methuselah96
Copy link
Contributor Author

@drcmda No problem, here you go: #2126.

@CodyJasonBennett
Copy link
Member

Should be easier to mirror the fix for v7 now. Thanks.

@CodyJasonBennett CodyJasonBennett merged commit 2928233 into pmndrs:master Mar 16, 2022
@Methuselah96 Methuselah96 deleted the add-react-reconciler-types-dependency branch March 16, 2022 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants