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: fix extensionless import for --type #61

Merged
merged 2 commits into from Mar 15, 2019

Conversation

Projects
None yet
4 participants
@MylesBorins
Copy link
Member

MylesBorins commented Mar 14, 2019

At some point this regressed for extensionless imports
for esm.

esm: fix extensionless import for --type
At some point this regressed for extensionless imports
for esm.
@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 14, 2019

@GeoffreyBooth
Copy link
Contributor

GeoffreyBooth left a comment

Not sure if this is related, but don’t forget that extensionless files are supposed to also look for the "type" field too, not only be executable via --type. The example we’d been discussing was TypeScript’s node_modules/typescript/bin/tsc, which is extensionless and could be written in ESM and executed without --type if node_modules/typescript/package.json contains "type": "module".

@MylesBorins

This comment was marked as outdated.

Copy link
Member Author

MylesBorins commented Mar 14, 2019

@GeoffreyBooth it currently only looks at the type field from the package.json, which in this implementation would trump the type flag (perhaps not what we want?)

edit: try to make more sense

before this change the --type flag was not, afaict, being respected. module_wrap.cc is only looking for package.type.

With this change we use the --type flag, but only if there is no package.type. Perhaps that is not expected behavior

@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Mar 14, 2019

Yes extensionless support for package.json "type" sounds like a separate bug. Perhaps we can tackle that separately.

Update: Actually it seems this is implemented fine already.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 14, 2019

Updated, I think it has the correct behavior right now

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 14, 2019

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 14, 2019

failure is unrelated. Will land in 2 hours if there are no objections... necessary for upstreaming

@ljharb

ljharb approved these changes Mar 14, 2019

@MylesBorins MylesBorins merged commit 98f4d99 into modules-lkgr Mar 15, 2019

@ljharb ljharb deleted the fix-extensionless branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.