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

fix(jest-runtime): fix node:-prefixed modules loading #11802

Conversation

antongolub
Copy link
Contributor

@antongolub antongolub commented Aug 31, 2021

relates #11331

Hey, @SimenB

Got trouble on loading node:-prefixed modules with Node.js 14 and jest v27.1.0:

https://github.com/antongolub/tsc-esm-fix/runs/3472541312?check_suite_focus=true




$ /home/runner/work/tsc-esm-fix/tsc-esm-fix/node_modules/.bin/jest -v
27.1.0


FAIL src/test/ts/index.ts

  ● Test suite failed to run

    Cannot find module 'node:fs'
    Require stack:
    - /home/runner/work/tsc-esm-fix/tsc-esm-fix/node_modules/jest-runtime/build/index.js
    - /home/runner/work/tsc-esm-fix/tsc-esm-fix/node_modules/@jest/core/build/cli/index.js
    - /home/runner/work/tsc-esm-fix/tsc-esm-fix/node_modules/@jest/core/build/jest.js
    - /home/runner/work/tsc-esm-fix/tsc-esm-fix/node_modules/jest/node_modules/jest-cli/build/cli/index.js
    - /home/runner/work/tsc-esm-fix/tsc-esm-fix/node_modules/jest/node_modules/jest-cli/bin/jest.js
    - /home/runner/work/tsc-esm-fix/tsc-esm-fix/node_modules/jest/bin/jest.js

    Require stack:
      node_modules/jest-runtime/build/index.js
      node_modules/@jest/core/build/cli/index.js
      node_modules/@jest/core/build/jest.js
      node_modules/jest/node_modules/jest-cli/build/cli/index.js
      node_modules/jest/node_modules/jest-cli/bin/jest.js
      node_modules/jest/bin/jest.js
      
at Runtime._requireCoreModule (node_modules/jest-runtime/build/index.js:1561:12)
 at Object.<anonymous> (node_modules/globby/index.js:20:38)

The current impl trims node: if the runtime already supports it.




private _requireCoreModule(moduleName: string, supportPrefix: boolean) {
  const moduleWithoutNodePrefix =
    supportPrefix && moduleName.startsWith('node:')
      ? moduleName.slice('node:'.length)
      : moduleName;

  if (moduleWithoutNodePrefix === 'process') {
    return this._environment.global.process;
  }

  if (moduleWithoutNodePrefix === 'module') {
    return this._getMockedNativeModule();
  }

  return require(moduleWithoutNodePrefix);
}

I suggest to invert this logic:

  private _requireCoreModule(moduleName: string, supportPrefix: boolean) {
    const moduleWithoutNodePrefix = moduleName.startsWith('node:')
      ? moduleName.slice('node:'.length)
      : moduleName;

    if (moduleWithoutNodePrefix === 'process') {
      return this._environment.global.process;
    }

    if (moduleWithoutNodePrefix === 'module') {
      return this._getMockedNativeModule();
    }

    return require(supportPrefix ? moduleName : moduleWithoutNodePrefix);
  }

@codecov-commenter
Copy link

Codecov Report

Merging #11802 (335f0a4) into master (98f10e6) will not change coverage.
The diff coverage is 75.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11802   +/-   ##
=======================================
  Coverage   69.04%   69.04%           
=======================================
  Files         312      312           
  Lines       16366    16366           
  Branches     4746     4747    +1     
=======================================
  Hits        11300    11300           
  Misses       5039     5039           
  Partials       27       27           
Impacted Files Coverage Δ
packages/jest-runtime/src/index.ts 59.14% <75.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98f10e6...335f0a4. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Aug 31, 2021

Our current behavior matches Node itself, though.

$ node -e 'console.log(process.versions.node); require("node:fs")'
14.17.5
internal/modules/cjs/loader.js:892
  throw err;
  ^

Error: Cannot find module 'node:fs'
Require stack:
- /Users/simen/[eval]
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:889:15)
    at Function.Module._load (internal/modules/cjs/loader.js:745:27)
    at Module.require (internal/modules/cjs/loader.js:961:19)
    at require (internal/modules/cjs/helpers.js:92:18)
    at [eval]:1:37
    at Script.runInThisContext (vm.js:134:12)
    at Object.runInThisContext (vm.js:310:38)
    at internal/process/execution.js:81:19
    at [eval]-wrapper:6:22
    at evalScript (internal/process/execution.js:80:60) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/simen/[eval]' ]
}

I don't think Jest should work when it would fail at runtime when you deploy it

@antongolub
Copy link
Contributor Author

antongolub commented Aug 31, 2021

Agree. But the code that I'm trying to test should be loaded with import syntax (esm module), not require.

node -e 'console.log(process.versions.node); (async() => await import("node:fs"))()' 
14.17.1

@SimenB
Copy link
Member

SimenB commented Aug 31, 2021

If you use ESM in your tests (https://jestjs.io/docs/ecmascript-modules) it should work already. Does it not?

@antongolub
Copy link
Contributor Author

antongolub commented Aug 31, 2021

Unfortunately, I have TS which is transpiled with ts-jest. As I can see, It somehow produces require for all kinds of module loading.

@SimenB
Copy link
Member

SimenB commented Sep 1, 2021

@antongolub
Copy link
Contributor Author

antongolub commented Sep 1, 2021

No luck. "useESM" is applied to the project's own codebase, not its deps. Anyway, I'll do some more experiments.

@SimenB
Copy link
Member

SimenB commented Sep 1, 2021

That's an issue with ts-jest, then, not Jest (more likely a configuration issue than a bug). Regardless of where, your code uses require at runtime and I do think it's correct behavior by Jest to respect how require works in Node as much as possible, so I'll close this.

@SimenB SimenB closed this Sep 1, 2021
@antongolub
Copy link
Contributor Author

antongolub commented Sep 1, 2021

@SimenB,

Hold on. I've removed TS, ts-jest and transforms that could be confusing. Just a pure ESM stand. Take a look: https://github.com/antongolub/jest-issue-11802

@antongolub
Copy link
Contributor Author

antongolub commented Sep 3, 2021

@SimenB,

I seem to have found the source of the problem. Here's the point where ESM flow breaks against the legacyrequire API.

// #289 node_modules/jest-resolve/build/resolver.js
        const resolvedModule =
          resolveNodeModule(module) || require.resolve(module);

Digging notes: https://github.com/antongolub/jest-issue-11802

@tusbar
Copy link

tusbar commented Sep 7, 2021

I have the same issue, without typescript as well, it seems to only fail when the node: import is in node_modules.

@antongolub
Copy link
Contributor Author

Oh, thanks for the reply. I really thought that I have the most crooked hands in the world)
I checked again: the issue occurs in Node.js <=16 when you invoke require API for node:-prefixed module at any level of the project's dep tree.

So even

// index.js

import fs from 'node:fs'
console.log('success')

is working with a direct node index.js invocation, but is failing with the latest v27.1.0 Jest.

@SimenB,
Have you been able to reproduce the bug?

@Kumartalan

This comment has been minimized.

@SimenB
Copy link
Member

SimenB commented Sep 7, 2021

@antongolub thanks for the minimal reproduction 👍 I've opened up a PR fixing it: #11817

@SimenB
Copy link
Member

SimenB commented Sep 8, 2021

@github-actions
Copy link

github-actions bot commented Oct 9, 2021

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 github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants