Skip to content
This repository has been archived by the owner on Apr 16, 2020. It is now read-only.

Replace --entry-type with --input-type, plus --input-type=auto #68

Conversation

GeoffreyBooth
Copy link
Member

#66 plus the addition of --input-type=auto. Same logic as #55: if the input string contains an import or export statement, treat it as ESM; otherwise as CommonJS. This is perhaps even more useful for --eval and --print.

I’m submitting this as a separate PR in case people are okay with #66 but uncomfortable with this addition. You can see the difference between the PRs here:

https://github.com/nodejs/ecmascript-modules/compare/flag-for-string-input-only…flag-for-string-input-only-with-auto

It’s the same code as from #55, without -a.

@GeoffreyBooth GeoffreyBooth added enhancement New feature or request modules-agenda labels Apr 4, 2019
@ljharb

This comment has been minimized.

@GeoffreyBooth GeoffreyBooth changed the base branch from modules-lkgr to flag-for-string-input-only April 4, 2019 06:38
@GeoffreyBooth

This comment has been minimized.

@GeoffreyBooth GeoffreyBooth force-pushed the flag-for-string-input-only-with-auto branch from 64c0a95 to 9dbcc5b Compare April 4, 2019 06:41
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Repeating my review from #55; If we can’t detect the type unambiguously from syntax, it should throw.

lib/internal/modules/esm/default_resolve.js Outdated Show resolved Hide resolved
@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Apr 4, 2019

Repeating my review from #55; If we can’t detect the type unambiguously from syntax, it should throw.

I’m not sure what this means. What are you proposing?

The definition of auto is in the docs in this PR: run as ESM if an import or export statement is detected, else run as CommonJS.

That’s perhaps not the holy grail of unambiguous syntax detection, but as we’ve discussed, unambiguous is impossible. Rather than try to come close and fail some of the time, this redefines the goal: import/export = ESM, else CommonJS. The user simply needs to know that that’s what “auto” means. That can be explained in a sentence, and isn’t all that complicated, so I think users can be trusted with this.

@GeoffreyBooth GeoffreyBooth force-pushed the flag-for-string-input-only-with-auto branch from f46476e to 52e8e20 Compare April 4, 2019 07:06
@GeoffreyBooth GeoffreyBooth force-pushed the flag-for-string-input-only branch from 391215a to 339ae10 Compare April 8, 2019 04:08
@GeoffreyBooth
Copy link
Member Author

Per 2019-04-10 meeting, I’ll refactor this into a separate PR that creates a new function on module, e.g. import { detectImportOrExportStatement } from 'module' or whatever it ends up being named. Then Node is providing this as an API for tools like babel and coffee to have a common, Node-supported way of picking a type to use for unknown JavaScript string input. The tools could use the result of that call in deciding whether to use vm.runScript or vm.runModule or whatever the recommended method is for them to run JavaScript code.

Once that’s done and merged in, we can discuss extending its use to other locations such as --input-type=auto.

@ljharb
Copy link
Member

ljharb commented Apr 10, 2019

(Let's make sure the API is usable from CJS as well 😄 )

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Apr 18, 2019

Closing in favor of #69

@GeoffreyBooth GeoffreyBooth deleted the flag-for-string-input-only-with-auto branch April 18, 2019 22:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request modules-agenda
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants