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 --experimental-modules main doesn't work (works only if extension supplied) #16476

Closed
giltayar opened this issue Oct 25, 2017 · 9 comments
Closed
Labels
cli Issues and PRs related to the Node.js command line interface. esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@giltayar
Copy link
Contributor

  • Version: v8.8.0
  • Platform: macOS
  • Subsystem: ES Modules

Assuming the simple file main.mjs:

console.log('hi')

Running node --experimental-modules main.mjs will work, but node --experimental-modules main won't work. You get:

{ Error: Cannot find module file:///********/main
    at module.exports (internal/loader/search.js:14:12)
    at exports.resolve (internal/loader/ModuleRequest.js:93:11)
    at Loader.resolve (internal/loader/Loader.js:51:40)
    at Loader.getModuleJob (internal/loader/Loader.js:79:40)
    at Loader.import (internal/loader/Loader.js:101:28)
    at module.js:454:29
    at Function.Module._load (module.js:455:7)
    at Function.Module.runMain (module.js:653:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3 code: 'MODULE_NOT_FOUND' }

This is true even if the extension of the file is js.

This is a regression - it works fine in v8.7.

If it's a feature and not a bug, then it doesn't make sense to me, as importing the same file without an extension works fine - and I believe running a file in the command line and importing it should have similar semantics.

@mscdex mscdex added cli Issues and PRs related to the Node.js command line interface. esm Issues and PRs related to the ECMAScript Modules implementation. labels Oct 25, 2017
@guybedford
Copy link
Contributor

guybedford commented Oct 26, 2017

To explain the logic here - the main module is resolved to an absolute URL (file://path/to/main) before being passed to the top-level import to go through the ES module resolver. This is because node x is different to import "x" (first is a file require where x is a path, second is a package require where x may be in node_modules).

Absolute paths are not passed through the file extension checks (.mjs, .js, .node, .json, ...) in the default NodeJS ES module resolver implemented (although I believe they are in require) in order to ensure that they form an identity map (I believe the argument here is for consistency with browser resolution), thus the extension doesn't get checked, only for relative resolve modes like /path/to/main, ./path/to/main or pkg/main.

If this is an ingrained enough behaviour that we should support it, then there are a few options here, but it may need some careful discussion.

@targos
Copy link
Member

targos commented Oct 26, 2017

@guybedford so the problem is that custom hooks should be able to change how the main entry point is resolved?

@giltayar
Copy link
Contributor Author

Note that given that this is also true for the .js extension, then once esm is out of the experiment phase, it will probably break a lot of scripts that rely on this behavior.

@targos
Copy link
Member

targos commented Oct 26, 2017

@guybedford do you see a problem with the following patch?

diff --git a/lib/module.js b/lib/module.js
index e92c0b61d9..abc8006c3a 100644
--- a/lib/module.js
+++ b/lib/module.js
@@ -435,7 +435,7 @@ Module._load = function(request, parent, isMain) {
           ESMLoader.hook(hooks);
         }
       }
-      await ESMLoader.import(getURLFromFilePath(request).href);
+      await ESMLoader.import(getURLFromFilePath(request).pathname);
     })()
     .catch((e) => {
       console.error(e);

@guybedford
Copy link
Contributor

@targos yes that’s probably the easiest patch, but does mean accepting the somewhat unique concept of the top main being an absolute path and not an absolute url for the resolver.

@guybedford
Copy link
Contributor

@targos I do think the patch above will be the best way forward here. Personally I think the most elegant solution would be supporting extension checking for full URLs, but I think that's a discussion for another time. Created #16526.

guybedford added a commit to guybedford/node that referenced this issue Oct 26, 2017
@giltayar
Copy link
Contributor Author

giltayar commented Oct 28, 2017

There's another nuance to this bug. It concerns files with no extensions.

This will work:

node ./node_modules/.bin/mocha

But this won't:

node --experimental-modules ./node_modules/.bin/mocha

It fails with ERR_UNKNOWN_FILE_EXTENSION. Which makes sense, given that it doesn't know whether it's a cjs or esm file.

Not sure how to resolve this, but I'm assuming that for reasons of backward compatibility, once we are out of the experiment, then this MUST work and resolve to CJS.

This is unfortunate, because a lot of people (including me!) use this "trick" of not having an extension to make it a Unix executable by adding a hash bang at the beginning, e.g.: #!/usr/bin/env node. Which begs the future question, how do we specify that such a file is ESM? Or do we just not do it and remove this capability from NodeJS?

@guybedford
Copy link
Contributor

@giltayar that pattern of node ./node_modules/.bin/mocha doesn't work on Windows due to the use of npm batch files to handle the indirection.

Rather just use ./node_modules/.bin/mocha here as recommended I think.

@targos targos closed this as completed in cadc47f Oct 28, 2017
@jdalton
Copy link
Member

jdalton commented Oct 28, 2017

@giltayar Would you open this as a new issue? While it's platform-ish specific I've seen it in the wild and having it error would be something that catches folks off guard. I think it deserves its own discussion in a new issue.

gibfahn pushed a commit that referenced this issue Oct 30, 2017
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: #16526
Fixes: #16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
gibfahn pushed a commit that referenced this issue Oct 30, 2017
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: #16526
Fixes: #16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
gibfahn pushed a commit that referenced this issue Oct 31, 2017
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: #16526
Fixes: #16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this issue Nov 2, 2017
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: nodejs/node#16526
Fixes: nodejs/node#16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this issue Nov 2, 2017
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: nodejs/node#16526
Fixes: nodejs/node#16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
The reason is that absolute URLs do not go through extension and index
checks. By switching to an absolute path, the resolver still applies
extensions properly to the top-level main.

PR-URL: nodejs/node#16526
Fixes: nodejs/node#16476
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issues and PRs related to the Node.js command line interface. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

No branches or pull requests

5 participants