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

fix: do not inject global variable into module wrapper #10644

Merged
merged 1 commit into from Nov 4, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 18, 2020

Summary

It's not part of the module wrapper, so we shouldn't inject it: https://nodejs.org/api/modules.html#modules_the_module_wrapper

Fixes #10565.

EDIT: Huh, seems like this blows up for tests I didn't bother to run locally. Specifically, it explodes for tests using global in JSDOM. I'm thinking we can just add it to the JSDOM env and remove in a future version. Might be considered a breaking change, though?

Test plan

Test added

@@ -366,7 +366,7 @@ class Runtime {
invariant(context);

if (this._resolver.isCoreModule(modulePath)) {
const core = await this._importCoreModule(modulePath, context);
const core = this._importCoreModule(modulePath, context);
Copy link
Member Author

@SimenB SimenB Oct 18, 2020

Choose a reason for hiding this comment

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

random fix - we wanna insert the module into the cache ASAP so we don't create multiple. The cache should only have promises anyways, so this was just an oversight (and would have been a type error if the ESM vm APIs had been typed)

@@ -29,7 +29,6 @@ export type ModuleWrapper = (
require: Module['require'],
__dirname: string,
__filename: Module['filename'],
global: Global.Global,
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

heh, as I wrote in an edit in the OP - it breaks for JSDOM as that does not have this code

@@ -45,6 +45,9 @@ class JSDOMEnvironment implements JestEnvironment {
throw new Error('JSDOM did not return a Window object');
}

// for "universal" code (code should use `globalThis`)
global.global = global;
Copy link
Member Author

Choose a reason for hiding this comment

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

the need for this makes it a breaking change as environments haven't had to do this before

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

extraGlobals should probably also be proper globals, put not for this PR I guess

@SimenB
Copy link
Member Author

SimenB commented Oct 19, 2020

extraGlobals should probably also be proper globals, put not for this PR I guess

That defeats (half) the purpose of it (#5163 (comment), specifically the part about 50% speedup)

@SimenB SimenB merged commit a82babd into jestjs:master Nov 4, 2020
@SimenB SimenB deleted the inject-global branch November 4, 2020 17:28
jeysal added a commit to mmkal/jest that referenced this pull request Nov 8, 2020
* master: (398 commits)
  chore(breaking): remove undocumented `enabledTestsMap` config (jestjs#10787)
  Change expect.not.objectContaining() to match documentation (jestjs#10708)
  chore: add name to root project (jestjs#10782)
  Added explanation on how to use custom @jest-environment to docs (jestjs#10783)
  fix: remove deprecated functions from the jest object (jestjs#9853)
  chore: convert jest-runtime to ESM (jestjs#10325)
  fix(resolve): use escalade to find package.json (jestjs#10781)
  feat(jest-runner): set exit code to 1 if test logs after teardown (jestjs#10728)
  chore: add `exports` field to all `package.json`s (jestjs#9921)
  fix: do not inject `global` variable into module wrapper (jestjs#10644)
  chore: migrate jest-resolve to ESM (jestjs#10688)
  chore(transform): refactor API to pass an options bag around rather than multiple boolean options (jestjs#10753)
  chore: default to node test env rather than browser (jestjs#9874)
  fix: drop support for node 13 (jestjs#10685)
  chore: show enhanced syntax error for all syntax errors (jestjs#10749)
  chore: update lockfile after publish
  v26.6.3
  chore: update changelog for release
  Don't throw an error if mock dependency can't be found (jestjs#10779)
  chore: bump babel core types (jestjs#10772)
  ...
@SimenB SimenB mentioned this pull request Apr 7, 2021
21 tasks
@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 May 11, 2021
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.

Jest does not allow constants called "global", not even in imported files.
3 participants