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

Node.js 15.9 regression with Jest ESM #37426

Closed
ai opened this issue Feb 18, 2021 · 19 comments
Closed

Node.js 15.9 regression with Jest ESM #37426

ai opened this issue Feb 18, 2021 · 19 comments

Comments

@ai
Copy link

ai commented Feb 18, 2021

After upgrading Node.js 15.8 → 15.9 Jest stopped supporting ESM.

  • Version: 15.9
  • Platform: Linux foxbat 5.10.15-200.fc33.x86_64 #1 SMP Wed Feb 10 17:46:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

What steps will reproduce the bug?

echo '{ "private": true, "type": "module" }' > package.json
yarn add nanoevents
echo "import { createNanoEvents } from 'nanoevents'" > index.js
echo "import './index.js'; it.todo('test')" > index.test.js
yarn add jest@next

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

volta install node@15.8
node --experimental-vm-modules node_modules/.bin/jest

(node:60479) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 PASS  ./index.test.js
  ✎ todo test

Test Suites: 1 passed, 1 total
Tests:       1 todo, 1 total
Snapshots:   0 total
Time:        0.173 s
Ran all test suites.

What do you see instead?

volta install node@15.9
node --experimental-vm-modules node_modules/.bin/jest

(node:60418) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 FAIL  ./index.test.js
  ● Test suite failed to run

    linking error, dependency promises must be resolved on instantiate

Additional information

@targos
Copy link
Member

targos commented Feb 18, 2021

/cc @devsnek

Could it be that #37300 did have an effect on the public API after all?

@SimenB
Copy link
Member

SimenB commented Feb 18, 2021

I assume this is a bug in Jest's implementation which is surfaced by some added check. I'm happy to fix that if I could get some pointers to what might be wrong to cause such a warning

@Trott
Copy link
Member

Trott commented Feb 19, 2021

Could it be that #37300 did have an effect on the public API after all?

I did a git bisect on this issue and it is indeed that commit where the problem was introduced.

521c08d

@Trott
Copy link
Member

Trott commented Feb 19, 2021

Somewhat related: Maybe let's see if we can land nodejs/citgm#728 to close nodejs/citgm#684?

@SimenB
Copy link
Member

SimenB commented Feb 19, 2021

Jest's tests are not failing on 15.9, so it wouldn't have caught this particular error.

But yes, should definitely try to land it in CITGM regardless 👍

@ai
Copy link
Author

ai commented Mar 3, 2021

15.11 still has an issue

@SimenB
Copy link
Member

SimenB commented Mar 3, 2021

I still think it's more likely to be a bug in Jest's implementation than anything, but I'm not sure where. I've had next to no time for OSS in the last few months unfortunately, so haven't found the time to dig into this

@akauppi
Copy link

akauppi commented Mar 4, 2021

Getting similar errors: linking error, not in local cache. akauppi/firebase-jest-testing#5

Likely the same reason. On node 15.10.0 (macOS + Homebrew).

Nice to see people are on this. Let me know if further (testing) assistance is appreciated.

@devsnek
Copy link
Member

devsnek commented Mar 4, 2021

i've never used jest before. how do i make it print a proper stack trace instead of just the error message?

@ai
Copy link
Author

ai commented Mar 4, 2021

@devsnek Node.js errors about ESM linking have no stack trace right now.

Jest prints stack trace if an error has it.

@devsnek
Copy link
Member

devsnek commented Mar 4, 2021

@ai ...yes they do

@ai
Copy link
Author

ai commented Mar 4, 2021

Sorry. My fault, I just found that I saw only ESM errors in Jest :(

@devsnek
Copy link
Member

devsnek commented Mar 4, 2021

Found the issue. Pretty cool race condition. Also reproduces without 521c08d.

Minimal repro:

const { SourceTextModule, SyntheticModule } = require('vm');

const tm = new SourceTextModule('import "a"');

tm
  .link(async () => {
    const tm2 = new SourceTextModule('import "b"');
    tm2.link(async () => {
      const sm = new SyntheticModule([], () => {});
      await sm.link(() => {});
      await sm.evaluate();
      return sm;
    }).then(() => tm2.evaluate());
    return tm2;
  })
  .then(() => tm.evaluate())
  .then(() => {
    console.log('dun');
  })
  .catch((e) => {
    console.error(e);
  });

Working on a fix....

@SimenB a good short-term fix is to only call link at the top level import, and let it automatically call the linker cb on the dependencies (and same with evaluate, you don't need to call it on each module)

devsnek added a commit to devsnek/node that referenced this issue Mar 4, 2021
This fixes an annoying bug where the entry module would link before its
dependencies were linked. This is fixed by forcing modules to wait for
all dependency linker promises to resolve.

Fixes: nodejs#37426

Signed-off-by: snek <me@gus.host>
devsnek added a commit to devsnek/node that referenced this issue Mar 4, 2021
This fixes an annoying bug where the entry module would link before its
dependencies were linked. This is fixed by forcing modules to wait for
all dependency linker promises to resolve.

Fixes: nodejs#37426

Signed-off-by: snek <me@gus.host>
@devsnek
Copy link
Member

devsnek commented Mar 4, 2021

Actually, I'm going to have to call this working as intended. It isn't possible to solve this without also breaking circular dependencies. Jest will need to refactor its code to not link before all modules in the tree are linked.

@devsnek devsnek closed this as completed Mar 4, 2021
@SimenB
Copy link
Member

SimenB commented Mar 4, 2021

Cool, thanks for the pointer @devsnek!

@SimenB a good short-term fix is to only call link at the top level import, and let it automatically call the linker cb on the dependencies (and same with evaluate, you don't need to call it on each module)

This applies only to static imports I assume? So for dynamic imports I'd still invoke link and evaluate manually? In that case, what about a dynamic import that has its own static import? Should I only call link and evaluate on the first import and then on dynamic imports, but let all static imports "do its own thing"?


i've never used jest before. how do i make it print a proper stack trace instead of just the error message?

For future reference, do return lines here (or rather in the associated transpiled .js version) instead of the filter: https://github.com/facebook/jest/blob/024295c49ec7ac5baa6de1ec1888ce29a5d0c35f/packages/jest-message-util/src/index.ts#L191

@SimenB
Copy link
Member

SimenB commented Mar 4, 2021

Ok, I think I have a fix. 🤞 All of Jest's existing ESM tests pass in addition to the reproduction in this bug at least. It also has a simplification of how we handle circular dependencies, so I have high hopes this is a better implementation than the one we're currently shipping. Thank you so much for the help @devsnek!

PR, for reference: jestjs/jest#11150

@targos
Copy link
Member

targos commented Mar 4, 2021

btw, are we ready to transition the VM modules API out of experimental?

@devsnek
Copy link
Member

devsnek commented Mar 4, 2021

@SimenB for both static and dynamic imports, you only have to call link and evaluate on the entrypoint. You could remove the isStatic distinction in jest entirely.

@SimenB
Copy link
Member

SimenB commented Mar 6, 2021

Yep, that's the approach I went for. Thanks again! 👍

btw, are we ready to transition the VM modules API out of experimental?

I know you didn't ask me, but I think #33439 and #35714 should be solved first (especially the latter as that might need an API change?)

EDIT: And probably #35889 and #36351 (which might be the same thing?)


I also still have hopes for #31658 being fixed at some point, but that doesn't really affect the VM ESM APIs so might not affect any stability/flagging decision

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants