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.

Sign up for GitHub

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

Jump to bottom

What's the reason @types are being added as dependencies as opposed to peerDependencies? #729

Closed
gajus opened this issue Oct 26, 2021 · 4 comments · May be fixed by #777
Closed

What's the reason @types are being added as dependencies as opposed to peerDependencies? #729

gajus opened this issue Oct 26, 2021 · 4 comments · May be fixed by #777

Comments

@gajus
Copy link

gajus commented Oct 26, 2021

react-hooks-testing-library/package.json

Lines 53 to 55 in ca13ed9

"@types/react": ">=16.9.0",
"@types/react-dom": ">=16.9.0",
"@types/react-test-renderer": ">=16.9.0",

This is causing multiple React declarations errors in our codebase, e.g.

node_modules/@testing-library/react-hooks/node_modules/@types/react/index.d.ts:3127:13 - error TS2717: Subsequent property declarations must have the same type.  Property 'datalist' must be of type 'DetailedHTMLProps<HTMLAttributes<HTMLDataListElement>, HTMLDataListElement>', but here has type 'DetailedHTMLProps<HTMLAttributes<HTMLDataListElement>, HTMLDataListElement>'.

3127             datalist: React.DetailedHTMLProps<React.HTMLAttributes<HTMLDataListElement>, HTMLDataListElement>;
                 ~~~~~~~~

  node_modules/@types/react/index.d.ts:3126:13
    3126             datalist: React.DetailedHTMLProps<React.HTMLAttributes<HTMLDataListElement>, HTMLDataListElement>;
                     ~~~~~~~~
    'datalist' was also declared here.
@mpeyper
Copy link
Member

mpeyper commented Oct 26, 2021 •
edited

This has been discussed before in #110 (my latest thoughts). It basically boils down to we don't want to force peer dependency warnings about types for non-TS users and by using very permissive types (we only restrict the minimum version, any newer version is allowed), the types are supposed to be flattened to the top.

In most cases, removing node_modules and package-lock.json and reinstalling resolves the issue, however if it doesn't npm ls @types/react-test-renderer might help you work out where the duplicate is coming from as there might be another dependency that isn't as permissive as ours. There is also a know issue with yarn's packages resolution (#610) to be aware of.

@ksjogo
Copy link

ksjogo commented Jan 10, 2022

Hey, we are currently looking into adding this library into our codebase and the functionality is great.
But we were surprised that the library was pulling in @types as dependencies. Its a flow codebase, so they aren't already present, but @testing-library/react-hooks would be the only dependent.

From previous experiences with other ts libraries in our codebase, we would have expected the types to be devDependencies.

@mpeyper
Copy link
Member

mpeyper commented Jan 11, 2022 •
edited

Hi @ksjogo. The main reason we use a dependency instead of a peerDependency is for folks like yourself, who do not want to install the types to silence the peerDependency warning NPM will spit out at you if you don't have it installed. Dependent types as just a devDependency (i.e. not listing them as a peerDependency) is not correct for libraries wanting to support TS properly.

We are a typescript library and as such our TS users expect our dependencies to be installed and as they form part of our public API then dependency is, in my opinion, the correct place for them and, in general, installing the types when they are not needed is not an issue.

That said, I'm strongly considering moving them into peerDependency and marking them as peerDependencyMeta optional as the vast majority of out users will already have the types installed anyway, and it would fix the yarn issue (#610) and stop this question from being asked so often.

@mpeyper mpeyper mentioned this issue Jan 11, 2022
Closed
@joshuaellis joshuaellis mentioned this issue Jan 18, 2022
Draft
4 tasks
@mpeyper
Copy link
Member

mpeyper commented Apr 10, 2022

This should be resolved in v8.0.0 where we no longer install type dependencies.

@mpeyper mpeyper closed this as completed Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BREAKING CHANGE: Update deps testing-library/react-hooks-testing-library
3 participants
@ksjogo @gajus @mpeyper