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 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 ES Modules label Dec 18, 2019
@bmeck bmeck requested review from jkrems and guybedford Dec 18, 2019
@bmeck

This comment has been minimized.

Copy link
Member Author

bmeck commented Dec 18, 2019

Copy link
Contributor

guybedford left a comment

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

This comment has been minimized.

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

This comment has been minimized.

@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Dec 29, 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.

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

This comment has been minimized.

bmeck added 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

This comment has been minimized.

Copy link
Member Author

bmeck commented Dec 31, 2019

Landed in baa3621

@bmeck bmeck closed this Dec 31, 2019
@bmeck bmeck mentioned this pull request Jan 2, 2020
2 of 2 tasks complete
BridgeAR added 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 added 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.