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: empty ext from pkg type/main doesnt affect format #31021

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Dec 18, 2019

BREAKING (kind of a bugfix? unclear)

This ensures files with unknown extensions like extension.unknown are not
loaded as CJS/ESM when imported as a main entry point and makes
sure that those files would maintain the same format even if loaded
after the main entrypoint.

Added tests for importing missing extension and unknown extension after entrypoint along the way.

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

This ensures files with unknown extensions like foo.bar are not
loaded as CJS/ESM when imported as a main entry point and makes
sure that those files would maintain the same format even if loaded
after the main entrypoint.
@bmeck bmeck added the esm Issues and PRs related to the ECMAScript Modules implementation. label Dec 18, 2019
@bmeck
Copy link
Member Author

bmeck commented Dec 18, 2019

CC: @nodejs/modules-active-members

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

One of the major features of the ES module resolver is being able to support new file extensions in future, which is reliant on the fact that we always throw for unknown file extensions.

Giving special treatment to isMain was how we did this while ensuring compatibility with existing bins.

Happy to flesh this out further, but I don't think we should lose that extension property for the ESM resolver.

doc/api/esm.md Show resolved Hide resolved
@bmeck
Copy link
Member Author

bmeck commented Dec 19, 2019

It seems like there might be something more that needs to be done for node ./entry-point.wasm to handle WASI in addition to the changes here. Asking around about how main() and import are setup for WASI and it seems that entry points have some special behavior. We should ensure that is handled but likely would be a follow on PR instead of on top this one.

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor

It seems like there might be something more that needs to be done for node ./entry-point.wasm to handle WASI in addition to the changes here. Asking around about how main() and import are setup for WASI and it seems that entry points have some special behavior. We should ensure that is handled but likely would be a follow on PR instead of on top this one.

We are likely still missing the proper work on how Web Assembly integrates into the module system. There might be Web Assembly headers in future that specify the top-level resolution (like package.json). There is also some work to allow Web Assembly start functions to themselves run instantiation (removing the need for JS to be primary entry). Node.js should definitely track whatever happens here as it stabilizes.

@nodejs-github-bot
Copy link
Collaborator

bmeck pushed a commit that referenced this pull request Dec 31, 2019
This ensures files with unknown extensions like foo.bar are not
loaded as CJS/ESM when imported as a main entry point and makes
sure that those files would maintain the same format even if loaded
after the main entrypoint.

PR-URL: #31021
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@bmeck
Copy link
Member Author

bmeck commented Dec 31, 2019

Landed in baa3621

@bmeck bmeck closed this Dec 31, 2019
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
This ensures files with unknown extensions like foo.bar are not
loaded as CJS/ESM when imported as a main entry point and makes
sure that those files would maintain the same format even if loaded
after the main entrypoint.

PR-URL: #31021
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
This ensures files with unknown extensions like foo.bar are not
loaded as CJS/ESM when imported as a main entry point and makes
sure that those files would maintain the same format even if loaded
after the main entrypoint.

PR-URL: #31021
Reviewed-By: Guy Bedford <guybedford@gmail.com>
GeoffreyBooth added a commit to GeoffreyBooth/node that referenced this pull request Jan 19, 2020
GeoffreyBooth added a commit to GeoffreyBooth/node that referenced this pull request Jan 20, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This ensures files with unknown extensions like foo.bar are not
loaded as CJS/ESM when imported as a main entry point and makes
sure that those files would maintain the same format even if loaded
after the main entrypoint.

PR-URL: #31021
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
codebytere pushed a commit that referenced this pull request Feb 17, 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>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants