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 @jest/types to dev dependencies #10937

Closed
wants to merge 1 commit into from
Closed

Conversation

fedyk
Copy link

@fedyk fedyk commented Dec 9, 2020

Summary

The @jest/types has @types/node as dependency. As a result, after installing the recent version of pretty-format, TypeScript thinks that my web app is nodejs. The main problem is the timers. In nodejs they return Timeout object. In browser it returns a number:

image

@facebook-github-bot
Copy link
Contributor

Hi @fedyk!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

It needs to be a dependency otherwise TypeScript compilation will fail.

I don't know how to best deal with globals in differing environments. This is an issue for TS to figure out I believe.

microsoft/TypeScript#33111
microsoft/TypeScript#37775

@orta is this something the TS team is looking into/have a good workaround for? The two issues I found didn't have much traffic

@orta
Copy link
Member

orta commented Dec 9, 2020

We've come up with a bunch of ideas, and keep dismissed them as infeasible at scale (either through complexity, or putting too much onus on individuals o line things up perfectly which won't happen).

https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c is the latest idea which hasn't been disproven yet - that said, I wouldn't bet on it landing anytime soon.

For the long term, I'd recommend something like this PR, more importantly I'd look into weakening the external 'user-facing' Jest types to remove any explicit node references if possible. The same conflict would exist inside electron, or react native apps too for example.

@SimenB
Copy link
Member

SimenB commented Dec 10, 2020

Thanks for the link @orta, that looks really promising! Having the references only affect the file it's in is how I thought it worked until I started getting issues with it 😅

Not sure how we can make our own types less exact though - main issue is where we expose e.g. fs.Stat, or we depend on some library which does it. That said, we already verify that the only allowed references is node*, so we could trivially disallow it in our own types (although migration would not be trivial).

*) https://github.com/facebook/jest/blob/ca47512c77e6a325bca734a35a4ad15a45dee8aa/scripts/buildTs.js#L78-L104

@SimenB
Copy link
Member

SimenB commented Mar 8, 2021

Would just using optional peer dep work? I assume TS will just explode due to a /// reference to node even though the file is not directly imported

@Nemikolh
Copy link

Nemikolh commented Mar 8, 2021

@SimenB I think it might work if the user's tsconfig has skipLibCheck set to true?

But if it doesn't then I think you are right, TS won't like it.

@jameswilddev
Copy link

Hmm. Is it not possible to just move @types/node to a devDependency? I figured that was what you were supposed to do. I'm having conflicts between Jest and another package.

@github-actions
Copy link

This pull request 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 Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants