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

jest-util pollutes types with own version of @types/node #8092

Closed
peterjuras opened this issue Mar 9, 2019 · 16 comments · Fixed by #8129
Closed

jest-util pollutes types with own version of @types/node #8092

peterjuras opened this issue Mar 9, 2019 · 16 comments · Fixed by #8129

Comments

@peterjuras
Copy link

🐛 Bug Report

As part of the migration to typescript, jest-util now has a dependency on @types/node.

This creates issues with code that will be written in a consuming TypeScript application, since some function types are modified (e.g. setTimeout). You can see more comments on similar issues here: DefinitelyTyped/DefinitelyTyped#21310

I propose to remove the @types/node dependency from the jest-util package, in order to not affect any code that is written by the user.

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

I was afraid something like this would be an issue. It's really unfortunate that TS allows the global namespace to be polluted by dependencies of dependencies

I propose to remove the @types/node dependency from the jest-util package, in order to not affect any code that is written by the user.

Then jest-util will not be consumable in TS since it will reference types that do not exist. How does other packages resolve this?

@peterjuras
Copy link
Author

How about having the types as a peer or optional dependency

@dubzzz
Copy link
Contributor

dubzzz commented Mar 12, 2019

@SimenB Isn't the @types/node supposed to be in devDependencies? Not 100% sure but I believe it will be OK as soon as the packaged version of the jest-util package is the transpiled one

@milesj
Copy link

milesj commented Mar 13, 2019

If Jest's public API (the compiled declaration files) rely on @types/node, then it must be a dependency, otherwise it can be a dev dependency. Otherwise, this forces the consumer to install @types/node manually, which non-TS users shoudn't have to do.

@dubzzz
Copy link
Contributor

dubzzz commented Mar 13, 2019

I believe that, in the case where jest-util uses types from the @types in its d.ts, going for peerDependencies or opt ones might be the right thing to do. In a way, types are only needed for TS users and are not really runtime constraints but just dev contraints.

EDIT

RXJS choice is to put it inside devDependencies:
https://github.com/ReactiveX/rxjs/blob/master/package.json

If you use rxjs with TypeScript you need to manually add @types/node to your package.json otherwise you will get error stacks as soon as you import it from a TypeScript file.

@SimenB
Copy link
Member

SimenB commented Mar 13, 2019

Yeah, I think we'll do that as well. Sindre Sorhus does the same thing when adding typings now. The error TSC throws is pretty clear. Anybody wanna send a PR? We can just add @types/node to the root package.json and remove it from the individual packages.

Note that this is just @types/node, all other @types/* should remain

@peterjuras
Copy link
Author

Hi @SimenB,

out of curiosity, can you clarify which approach you would like to take? dubzzz's proposal?

@SimenB
Copy link
Member

SimenB commented Mar 13, 2019

@types/node should only be in the package.json in the root of the repo, nowhere else

@dubzzz
Copy link
Contributor

dubzzz commented Mar 13, 2019

Do you mean in devDependencies? Like rxjs does.

@SimenB
Copy link
Member

SimenB commented Mar 13, 2019

The root package.json is private, so it doesn't matter if it's dev or not. It's never published

@peterjuras
Copy link
Author

Is there a plan to publish a new jest version which includes this change?

@SimenB
Copy link
Member

SimenB commented Mar 30, 2019

There'll be a release next week

@SimenB
Copy link
Member

SimenB commented Apr 2, 2019

https://github.com/facebook/jest/releases/tag/v24.6.0

@Nemikolh
Copy link

Nemikolh commented Mar 8, 2021

Hi @SimenB, it looks like #10248 went backward in regard to this issue. Was it intentional?

I'm asking because we are thinking of upgrading jest but we get tons of type error when trying to do so. I'm not really sure what we can do, apart from having our front-end tests in their own project and ignore typescript compile errors there.

Many thanks,

@SimenB
Copy link
Member

SimenB commented Mar 8, 2021

See #10937 for latest status

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants