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

module: drop support for extensionless files in ESM #31415

Closed

Conversation

GeoffreyBooth
Copy link
Member

This PR removes support for extensionless files in ES module contexts, per discussion in #31388 (comment). The current ESM implementation that shipped in 12.0.0 supported extensionless files only as main entry points to the Node process, e.g. node extensionless-file, and not via import; support for extensionless files in import was added only last month, in #31021. This PR reverts that one, and goes further to drop support for extensionless files as main entry points. The docs are also updated.

The prior support for extensionless main entry files was there as part of supporting bin files. As a practical matter, the “empty string” extension was designated as ES module JavaScript (equivalent to .mjs) within a "type": "module" scope. This is problematic for adding future support for extensionless files of other formats, such as WASM/WASI.

In the interest of preserving design space for such future support, this PR makes all extensionless files unknown (and therefore throwing) to the ESM loader. Another solution besides #31021 or the implementation’s earlier special casing of extensionless main entry points will need to be found for configuring Node’s ES module loader on how to treat extensionless files of varying formats.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

cc @nodejs/modules-active-members

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Jan 20, 2020
@GeoffreyBooth GeoffreyBooth added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Jan 20, 2020
@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor

Note that it is still possible to require extensionless files with this PR though:

require('./file/without-extension');
import('./file/without-extension');

In order to disallow CommonJS execution from allowing ES module imports, we should add an extension filter to the "pre-emptive caching" at https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L1072.

@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member Author

Note that it is still possible to require extensionless files

To be clear, I think he means that once an extensionless file has been required, then import of that file will succeed when it shouldn’t, because of this preemptive caching. I think fixing that should be its own PR.

@aduh95
Copy link
Contributor

aduh95 commented Jan 20, 2020

To be clear, landing this PR means node ./bin/js-file-without-extension will be treated as CJS, no matter the package.json's type value, correct?

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Jan 20, 2020

To be clear, landing this PR means node ./bin/js-file-without-extension will be treated as CJS, no matter the package.json's type value, correct?

No. If it's in a type: module scope, an error will be thrown.

If that's your situation, you could create a ./bin/package.json containing type: commonjs, and then the extensionless file in ./bin will be treated as CommonJS and become executable.

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this alleviates prior concerns of edge cased behavior and solves the problem of removing edge cases that caused #31021 to be seen as a fix by taking the alternate route. Historically when behavior lacks consensus we remove controversial parts so this PR is likely the best route forward.

@bmeck bmeck mentioned this pull request Jan 20, 2020
4 tasks
@nodejs-github-bot
Copy link
Collaborator

@jkrems
Copy link
Contributor

jkrems commented Jan 20, 2020

I'm a bit bummed that this will mean that self-contained binaries at known locations will be stuck with CommonJS or symlinks forever. But not bummed enough to block this. :)

@GeoffreyBooth
Copy link
Member Author

this will mean that self-contained binaries at known locations will be stuck with CommonJS or symlinks forever.

Not forever, just until we find an alternative solution. 😄 There are options.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2020
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth added a commit that referenced this pull request Jan 23, 2020
reverses baa3621

PR-URL: #31415
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
GeoffreyBooth added a commit that referenced this pull request Jan 23, 2020
PR-URL: #31415
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
GeoffreyBooth added a commit that referenced this pull request Jan 23, 2020
PR-URL: #31415
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@GeoffreyBooth
Copy link
Member Author

Landed in dc90f92...c692568

@GeoffreyBooth GeoffreyBooth deleted the drop-import-of-extensionless branch January 23, 2020 07:22
guybedford pushed a commit to GeoffreyBooth/node that referenced this pull request Apr 1, 2020
PR-URL: nodejs#31415
Backport-PR-URL: nodejs#32280
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
Backport-PR-URL: #32280
PR-URL: #31415
Backport-PR-URL: #32280
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Apr 3, 2020
@Ryuzaki42
Copy link

What about .ts as file extension? I'm trying to import some typescript module w/o .js files

node --loader ts-node/esm --experimental-specifier-resolution=node src/index.ts

and got

TypeError [ERR_INVALID_RETURN_PROPERTY_VALUE]: Expected string to be returned for the "format" from the "loader getFormat" function but got type object.
    at new NodeError (node:internal/errors:278:15)
    at Loader.getFormat (node:internal/modules/esm/loader:111:13)
    at Loader.getModuleJob (node:internal/modules/esm/loader:231:20)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:58:21)
    at async Promise.all (index 1)
    at link (node:internal/modules/esm/module_job:63:9)

@iambumblehead

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. modules-agenda Issues and PRs to discuss during the meetings of the Modules team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet