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

esm: Bug in dynamic modules refactoring #25482

Closed
guybedford opened this issue Jan 13, 2019 · 7 comments
Closed

esm: Bug in dynamic modules refactoring #25482

guybedford opened this issue Jan 13, 2019 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs.

Comments

@guybedford
Copy link
Contributor

Reposted from nodejs/help#1717.

Specs:

  • Node.js Version:
    • Tested and working:
      • v11.3.0
    • Tested and broken:
      • v11.4.0
      • v11.5.0
  • OS: Tested on Linux Mint 19.1 Cinnamon
    • Linux HOSTNAME 4.15.0-43-generic #46-Ubuntu SMP Thu Dec 6 14:45:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
  • Scope (install, code, runtime, meta, other?): ES6 Module Import Regression from v11.3.0 to v11.4.0 using --experimental-modules
  • Module (and version) (if relevant): request-promise-native, but stacktrace also points to psl and core node as suspect.

Problem Statement:

Importing request-promise-native using ES6 module syntax on v11.4.0 or v11.5.0 causes a stacktrace which (I think) points to an error in core node:

kevin@rayquaza:~/tmp$ node --experimental-modules index.mjs 
(node:13310) ExperimentalWarning: The ESM module loader is experimental.
TypeError: Cannot read property 'onReady' of undefined
    at Module.load (internal/modules/cjs/loader.js:631:22)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:659:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/home/kevin/tmp/node_modules/psl/index.js:14:19)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:734:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)

Steps To Reproduce:

  • Be on node version v11.4.0 or later:
    • nvm install v11.4.0
  • Clone (or otherwise duplicate) the project from this gist: https://gist.github.com/kj800x/f08f44305429bbf8fdd65f9991cb2a71
    • git clone https://gist.github.com/kj800x/f08f44305429bbf8fdd65f9991cb2a71 && cd f08f44305429bbf8fdd65f9991cb2a71
    • It consists of an index file that imports request-promise-native and a package.json and a package-lock.json
  • Install dependencies:
    • npm i
  • Run the code
    • npm start
      • ACTUAL: Observe the stacktrace and error
  • Switch to node version v11.3.0
    • nvm install v11.3.0
  • Run the code
    • npm start
      • EXPECTED: Observe that you are able to successfully import the module without errors.

Related

The last three comments on this request-promise-native issue are related: request/request-promise-native#1 (comment)

@mririgoyen
Copy link

This issue is also happening in the 10 LTS version, breaking in 10.15.3. See #26595.

@paul-maxime
Copy link

I can confirm, we got the same error when trying to update from 10.15.1 to 10.15.3.

@jaydenseric
Copy link
Contributor

jaydenseric commented Mar 19, 2019

This also triggers the bug (Node.js v10.15.2 ok, v10.15.3 buggy):

import blockchainWalletService from 'blockchain-wallet-service'

See #25310 (comment).

@Fixten
Copy link

Fixten commented Apr 24, 2019

Was hoping there would be some progress with 12.x release. Are there any plans to fix it in the near future?

@aduh95
Copy link
Contributor

aduh95 commented Apr 29, 2019

I am able to reproduce the bug, I think I am able to isolate it: https://github.com/nodejs/node/pull/27443/files

Steps to reproduce:
Execute the file with the --experimental-modules flag => crashes
Execute the file without the --experimental-modules flag => works fine

Unfortunately I didn't find a successful fix, I feel I don't have enough experience on the CJS loader to understand the bug. I am leaving my test available for anyone who wants to work on that.

aduh95 added a commit to aduh95/node that referenced this issue May 2, 2019
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: nodejs#25482
@robsonrosa
Copy link

This issue is also happening in the 12.2.0 version.

@pandres95
Copy link

This issue is also happening in the 12.2.0 version.

Also happening in the 12.1.0 version.

ZYSzys pushed a commit to zys-contrib/node that referenced this issue May 13, 2019
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: nodejs#25482
@ZYSzys ZYSzys closed this as completed in 6fe3692 May 14, 2019
ZYSzys pushed a commit that referenced this issue May 14, 2019
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: #25482

PR-URL: #25491
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue May 14, 2019
Fixes: #25482

PR-URL: #25491
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue May 14, 2019
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: #25482

PR-URL: #25491
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Jun 6, 2019
Fixes: #25482

Backport-PR-URL: #27874
PR-URL: #25491
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Jun 6, 2019
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: #25482

Backport-PR-URL: #27874
PR-URL: #25491
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
Fixes: nodejs/node#25482

PR-URL: nodejs/node#25491
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: nodejs/node#25482

PR-URL: nodejs/node#25491
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants