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
Preserve import order for ESM #15031
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I didn't know Node did that 😅
This should be behind a flag, yes 👍 It also needs integration tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, wrong button 😅
(also changelog entry)
@SimenB Thanks for the response, it turns out this is a bit more nuanced than I thought - though the cause and the solution I believe are the same, I was only able to repro this by mixing ESM and CJS. I ended up using almost the exact repro that my project had (albeit simpler) I'm confident there are other situations it would appear in too. It ended up being more complicated than I thought to add the |
181caf6
to
f079261
Compare
a248dc8
to
d70ff0d
Compare
@SimenB tests passing and changelog provided - it was fairly cumbersome adding the new option (the same change required in lots of files) please let me know if I got it wrong |
Yeah I was caught out by that in #15016 - I do wonder if it might be worth trying to colocate some of the changes or have a template-ish doc outlining the different places that should be changed. fwiwi it looks like you've just missed updating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding tests!
Could you update the docs as well? Add the new option to https://jestjs.io/docs/configuration
@SimenB tests actually passing now 🤣 and I believe I updated the documentation too (though wasn't sure exactly which file relates to the website) |
@SimenB don't review this yet - just found that when you have circular dependencies jest now hangs because of this change - looking into it |
@SimenB this is ready for your re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some quick comments - I haven't looked at the e2e test yet, which I wanna do 🙂
packages/jest-runtime/src/index.ts
Outdated
@@ -737,12 +740,70 @@ export default class Runtime { | |||
) { | |||
return this.loadEsmModule(resolved, query); | |||
} | |||
|
|||
if (skipCjsModules) { | |||
// @ts-expect-error - exiting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add | undefined
to the return type instead? or null
possibly to be more explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do, not sure how cascading that will be though (e.g., TS errors on all the callsites) - it can only be null in this one case, which is only called from line 764, this was also the "established pattern" from line 594.
// this is a cyclic dependency. As such, everything between where | ||
// that module is in the parentChain and the end of the chain | ||
// (e.g., the current module) is also cyclic. | ||
const chainInOrder = [...parentChain]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is order in map guaranteed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more relevant is if Set
order is guaranteed - in both cases (set and map) yes, they are.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set
packages/jest-runtime/src/index.ts
Outdated
const mod = await this.resolveModule<VMModule>( | ||
specifier, | ||
referencingModule.identifier, | ||
referencingModule.context, | ||
); | ||
const hasCyclicDependency = await this.checkForCyclicDependencies( | ||
mod, | ||
parentChain, | ||
); | ||
if (mod.status === 'unlinked' && !hasCyclicDependency) { | ||
await this.linkAndEvaluateModule( | ||
mod, | ||
new Set<VMModule>([...parentChain, mod]), | ||
); | ||
} | ||
return mod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you extract this into a separate method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
certainly!
let linkPromiseChain = (Promise<VMModule | void>).resolve(); | ||
if (preserveLoadOrder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit more advanced than it needs to. Is there any reason not to unwrap this with something like
diff --git i/packages/jest-runtime/src/index.ts w/packages/jest-runtime/src/index.ts
index f4ff5238a7..154cd877bb 100644
--- i/packages/jest-runtime/src/index.ts
+++ w/packages/jest-runtime/src/index.ts
@@ -801,42 +801,32 @@ export default class Runtime {
return hasCyclicDependency;
}
- private linkModule(
+ private async linkModule(
specifier: string,
referencingModule: VMModule,
parentChain: Set<VMModule>,
- ): VMModule {
+ ): Promise<VMModule> {
+ const mod = await this.resolveModule<VMModule>(
+ specifier,
+ referencingModule.identifier,
+ referencingModule.context,
+ );
+
const preserveLoadOrder =
this._config.preserveLoadOrder || this._globalConfig?.preserveLoadOrder;
- // ensure that every import fully evaluates before any siblings are allowed to import.
- let linkPromiseChain = (Promise<VMModule | void>).resolve();
if (preserveLoadOrder) {
- linkPromiseChain = linkPromiseChain.then(async () => {
- const mod = await this.resolveModule<VMModule>(
- specifier,
- referencingModule.identifier,
- referencingModule.context,
- );
- const hasCyclicDependency = await this.checkForCyclicDependencies(
- mod,
- parentChain,
- );
- if (mod.status === 'unlinked' && !hasCyclicDependency) {
- await this.linkAndEvaluateModule(
- mod,
- new Set<VMModule>([...parentChain, mod]),
- );
- }
- return mod;
- });
- return linkPromiseChain;
+ // ensure that every import fully evaluates before any siblings are allowed to import.
+ const hasCyclicDependency = await this.checkForCyclicDependencies(
+ mod,
+ parentChain,
+ );
+ if (mod.status === 'unlinked' && !hasCyclicDependency) {
+ await this.linkAndEvaluateModule(mod, new Set([...parentChain, mod]));
+ }
}
- return this.resolveModule<VMModule>(
- specifier,
- referencingModule.identifier,
- referencingModule.context,
- );
+
+ return mod;
}
private async linkAndEvaluateModule(
?
for perf, skipping checkForCyclicDependencies
if mod.status !== 'unlinked'
probably makes sense as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it can be made simpler - the linkPromiseChain
is necessary:
import './file1.js';
import './file2.js';
without it, these two will call linkAndEvaluateModule
in parallel. If file2.js
depends on something was setup by file1.js
, it won't work. Similarly, resolveModule
must be called within the chain because the issue is with cjs vs esm modules - so something setup by esm and depended on by cjs must be available at the time resolveModule
is called - since, in the case of cjs, resolveModule
also evaluates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first check in checkForCyclicDependencies
is for mod.status !== 'unlinked'
- so all we'd save is the cost of the call itself, but it's an easy enough change to make
Summary
Ensure Jest behaves the same was as Node when loading ESM modules - by linking and evaluating up the import tree - rather than fully linking and evaluating asynchronously.
I'm aware this is likely a divisive, opinionated change - I'd be totally fine with this behaviour being hidden behind a flag. If this has already been suggested and rejected - I apologies, I did look for similar issues/PRs but found none.
In many projects (including some of those I'm involved with) the code depends on load order, which undeniably it shouldn't. However it isn't always trivial to clean up these implicit depenencies - particularly where globals are involved, defined in third party packages. When running this project in node, imports are fully evaluated before their siblings are loaded - unless top level
await
is used.However, when running jest - imports are linked and evaluated in parallel - while this is completely correct ESM behaviour, it massively differs from node's behaviour, making jest hard to use in many large, legacy projects with complex dependency trees (any of which may fall victim to this).
A potential repro would be:
In node, this will output
Hello
- in jest, it will outputundefined
- because file2 evaluates before file1 does. Note - I've not tested this case specifically, where I observed it was more complex - but the concept is the same.Test plan
I've tested this on a complex project - if necessary I can throw together a repro. Without this change, jest will log
undefined
with the change it will logHello