Skip to content

Node.js 15.9 regression with Jest ESM #37426

Description

@ai

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

Activity

  1. targos commented on Feb 18, 2021

    @targos
    Member

    /cc @devsnek

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

  2. SimenB commented on Feb 18, 2021

    @SimenB
    Member

    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

  3. Trott commented on Feb 19, 2021

    @Trott
    Member

    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

  4. Trott commented on Feb 19, 2021

    @Trott
    Member

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

  5. SimenB commented on Feb 19, 2021

    @SimenB
    Member

    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 👍

  6. ai commented on Mar 3, 2021

    @ai
    Author

    15.11 still has an issue

  7. SimenB commented on Mar 3, 2021

    @SimenB
    Member

    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

  8. akauppi commented on Mar 4, 2021

    @akauppi

    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.

  9. devsnek commented on Mar 4, 2021

    @devsnek
    Member

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

  10. ai commented on Mar 4, 2021

    @ai
    Author

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

    Jest prints stack trace if an error has it.

  11. devsnek commented on Mar 4, 2021

    @devsnek
    Member

    @ai ...yes they do

  12. ai commented on Mar 4, 2021

    @ai
    Author

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

  13. devsnek commented on Mar 4, 2021

    @devsnek
    Member

    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)

  14. devsnek commented on Mar 4, 2021

    @devsnek
    Member

    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.

  15. SimenB commented on Mar 4, 2021

    @SimenB
    Member

    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

  16. SimenB commented on Mar 4, 2021

    @SimenB
    Member

    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

  17. targos commented on Mar 4, 2021

    @targos
    Member

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

  18. devsnek commented on Mar 4, 2021

    @devsnek
    Member

    @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.

  19. SimenB commented on Mar 6, 2021

    @SimenB
    Member

    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions