fix: do not inject global variable into module wrapper
#10644
Conversation
| @@ -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); | |||
There was a problem hiding this comment.
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, | |||
There was a problem hiding this comment.
global is already defined (https://github.com/facebook/jest/blob/e84a70c8456d61daa36b5c79a4df7ee752d6d6d6/packages/jest-environment-node/src/index.ts#L34), so this isn't breaking
There was a problem hiding this comment.
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; | |||
There was a problem hiding this comment.
the need for this makes it a breaking change as environments haven't had to do this before
That defeats (half) the purpose of it (#5163 (comment), specifically the part about 50% speedup) |
* 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) ...
|
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. |
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
globalin 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