jest icon indicating copy to clipboard operation
jest copied to clipboard

Preserve import order for ESM

Open znewsham opened this issue 1 year ago • 9 comments

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:

// file1.js
import './emptyFile.js'
globalThis.whatever = "Hello";

// file2.js
console.log(globalThis.whatever);

// file3.js
import "./file1.js";
import "./file2.js";

In node, this will output Hello - in jest, it will output undefined - 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 log Hello

znewsham avatar Apr 18 '24 21:04 znewsham

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: znewsham / name: Zack (2ee1bf73e3c49a4b077060b18cecc766444e0676, f079261cea941ae5da0a92e93fd97d04de060a3f, cd817fb6700176eef5fda9b06746381b45001c3a, d70ff0dd47c6652401c6b61727053b13847feeb3, 733708074945fb2adc487826880d022b7818e15f, 10a90090bef6efa3f2974b2f6cb44d455a08f7be, 05c1a93c5fe019b0b7f83632dca62722f3f577fa, be8978dfa256b8b26c2cc968491cf0f30cdf1553, c8d5cebde6a1d9d58cb09378fcc08f5b34db46ed, c204184c8fd985c126ec5707d0eb720b4383d7a0, 61d49d66bb7bae9b19e0b5c03703fbb5cac02965, bc6deabdef5cac7de54124b9d407793c5738d44c, b602925c4c8076d80e70df6ea1323ac58a0c2d3b, 17131390281cc4768d0a79909aac5e7eb00c26da, 4e5fba3097976ad7345cb4d7c2aa2c8d6ee2235c, b3c788393628e2f0df2af859054c869c3fc396fe, 4cfcf6fd470fbec5617fbfc27f3e58359f1cf103)

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
Latest commit c204184c8fd985c126ec5707d0eb720b4383d7a0
Latest deploy log https://app.netlify.com/sites/jestjs/deploys/66423508dbf2f90008d6534a
Deploy Preview https://deploy-preview-15031--jestjs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Apr 18 '24 21:04 netlify[bot]

@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 preserveLoadOrder flag - please let me know if I've done it totally wrong

znewsham avatar Apr 19 '24 14:04 znewsham

@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

znewsham avatar Apr 25 '24 20:04 znewsham

it was fairly cumbersome adding the new option

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 jest-config/src/Descriptions.ts which should have a description of your new config option; that'll also result in needing to update a create-jest snapshot. There is also jest/cli/src/args.ts but from what I understand of this change it doesn't seem like something that would make much sense to be arg-based i.e. it feels like something you'd either want to configure as always on or always off.

G-Rath avatar Apr 26 '24 02:04 G-Rath

@SimenB tests actually passing now :rofl: and I believe I updated the documentation too (though wasn't sure exactly which file relates to the website)

znewsham avatar Apr 26 '24 16:04 znewsham

@SimenB don't review this yet - just found that when you have circular dependencies jest now hangs because of this change - looking into it

znewsham avatar Apr 29 '24 15:04 znewsham

@SimenB this is ready for your re-review

znewsham avatar May 09 '24 19:05 znewsham

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Aug 12 '24 15:08 github-actions[bot]

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

github-actions[bot] avatar Sep 11 '24 16:09 github-actions[bot]

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

github-actions[bot] avatar Sep 11 '24 16:09 github-actions[bot]

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[bot] avatar Oct 12 '24 00:10 github-actions[bot]