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

chore: use globalThis over global everywhere #12447

Merged
merged 5 commits into from
Feb 21, 2022
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 21, 2022

Summary

globalThis works in all environments, not just Node.

Vaguely related to #11569.

Test plan

Green CI.

@@ -35,6 +35,7 @@ const createState = (): Circus.State => {
};
};

/* eslint-disable no-restricted-globals */
Copy link
Member Author

Choose a reason for hiding this comment

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

mostly for types, not sure how to enhance globalThis with a symbol

@IanVS
Copy link
Contributor

IanVS commented Mar 10, 2022

I don't suppose there's any chance this could be back-ported to the 27 branch, could it? Using global is causing problems for storybook projects that use vite, now that jest-mock is included in each new storybook project and is running in the browser, and vite does not polyfill global. Dropping old node support (and even dropping most versions of 16), make it unlikely we'll be able to use 28 for a while.

@SimenB
Copy link
Member Author

SimenB commented Mar 10, 2022

Yep, globalThis doesn't exist in Node 10, so doing so would be a breaking change (unless checking if it exists and falling back to some polyfilled thing, which is a bit too much work). Also, releasing from non-HEAD is super painful.


I'd suggest locking the version to a known good one for your consumers (jest-mock's node version requirement isn't really that high, it just follows in lockstep with other packages in this repo), or just run some transform on v27 that replaces global with globalThis, and bundling it.

@IanVS
Copy link
Contributor

IanVS commented Mar 10, 2022

Thanks for responding. I'll talk to the team about the options, but it seems odd that Jest 28 will allow node 12.13+, but not node 16 less than 16.13. That feels overly restrictive, and I'm not clear for the reasoning there. Were features added in 16.13 that jest uses? But then, how would 12.13 work? Is it really necessary to set engines to prevent node 16.12 and below (https://github.com/facebook/jest/pull/12220/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R148)? I think jest 28 would be an easier sell to my team if it were just dropping node 10.

Thanks again for the suggestions.

@SimenB
Copy link
Member Author

SimenB commented Mar 10, 2022

It's the first LTS release of v16: https://nodejs.org/en/blog/release/v16.13.0/ (same with our minimal v12 and v14 - first LTS)

@IanVS
Copy link
Contributor

IanVS commented Mar 10, 2022

But what is the reason to require that as a minimum in this package? From my understanding, that's just the version when they started calling it "LTS". It seems like a bummer to prevent folks who may be on a fairly-up-to-date node.js version from using jest just because of an arbitrary label. It seems like engines should only be used to specify which versions of node are required in order to run jest (meaning, it won't work on anything else). But, this isn't really the place to discuss it I suppose.

@SimenB
Copy link
Member Author

SimenB commented Mar 11, 2022

The reason is that the less platforms we support, the easier maintenance of the project as a whole is. LTS releases are nice targets, and people valuing stability should be on those versions. If an issue is found with e.g. 16.10 which is fixed in 16.11, I have zero interest in fixing that or receiving bug reports about it.

16.14.0 release is close to 5 months old, I doubt it's a real issue for people wanting to run it. I highly doubt it's normal with people unable to upgrade their node versions, but are stuck on versions of node before they got the LTS stamp.

@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 Apr 11, 2022
@SimenB
Copy link
Member Author

SimenB commented Apr 27, 2022

@IanVS fwiw Jest 28.0.2 dropped node 16 req to 16.10. Dunno if that's in range of what you need, but thought to call it out 🙂 Might not help if your current range is something like >=16, though

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.

3 participants