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

Experimental module resolution inconsistency between --eval and file #28160

Closed
Avaq opened this issue Jun 10, 2019 · 6 comments
Closed

Experimental module resolution inconsistency between --eval and file #28160

Avaq opened this issue Jun 10, 2019 · 6 comments
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features.

Comments

@Avaq
Copy link

Avaq commented Jun 10, 2019

  • Version: v12.3.1
  • Platform: Linux thinkpad-avaq 4.19.47 #1-NixOS SMP Fri May 31 13:46:35 UTC 2019 x86_64 GNU/Linux

Preparation

  1. Go to a temporary directory: cd $(mktemp -d)
  2. Install Fluture or any other module that has a "main" field without file extension, and contains a .js main file as well as a .mjs main file: npm install fluture@11.0.1

Reproducing

Run the following two shell scripts:

node --input-type=module \
     --experimental-modules \
     --eval 'import Future from "fluture"; console.log(Future)'
echo 'import Future from "fluture"; console.log(Future)' > index.mjs
node --experimental-modules index.mjs

Expected result

The output of both shell scripts is the same.

Actual result

The first script errors with Error: Cannot find package 'fluture' imported from, and the output from the second shows that the module resolver loaded the CommonJS version of the package into the module.

/cc @ljharb

@ljharb
Copy link
Member

ljharb commented Jun 10, 2019

cc @nodejs/modules

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jun 10, 2019

the output from the second shows that the module resolver loaded the CommonJS version of the package into the module.

This is expected behavior. If Fluture’s package.json lacks "type": "module", then "main" will be read in CommonJS mode (unless it refers to an .mjs file and the extension is included in the "main" field’s value) and therefore "index" will be expanded to index.js. Fluture as currently designed is expecting the Node 8-11 design of --experimental-modules. See #27957.

The first script errors with Error: Cannot find package 'fluture' imported from

This I think is a bug, since the following prints an inspection of Fluture:

node --experimental-modules --input-type=commonjs \
     --eval 'const Fluture = require("fluture"); console.log(Fluture)'

The expected behavior for your first shell command (node --input-type=module...) would be the same as this --input-type=commonjs command, where the CommonJS version of Fluture is printed.

@guybedford
Copy link
Contributor

So the reason for the bug here is likely due to the fact that the evaluated module is given a unique ID eval:0 in the registry (https://github.com/nodejs/node/blob/master/lib/internal/modules/esm/loader.js#L110). So that any package resolutions are taken relative to this path and fail (no eval:node_modules!).

The fix would likely be to change this default ID form to something cwd-like so eg - file:///path/to/cwd/@eval?id or similar.

It would literally be that exact one liner at that link linked to above to fix!

@jkrems
Copy link
Contributor

jkrems commented Jun 16, 2019

The fix would likely be to change this default ID form to something cwd-like so eg - file:///path/to/cwd/@eval?id or similar.

That sounds like the same overall approach that is used for CommonJS:

$ node -p 'module.filename'
/Users/jkrems/code/src/github.com/jkrems/test/[eval]

@devsnek devsnek added confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. labels Jun 16, 2019
@zero1five
Copy link
Contributor

The test to fix this should be whether the context path for the current module exists.

@guybedford
Copy link
Contributor

This has been fixed in #28389.

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. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants