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: Implement esm mode flag #18392

Closed
wants to merge 5 commits into from
Closed

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Jan 26, 2018

This provides a mode: "esm" flag for package.json files which will treat ".js" files as ES modules within a given package boundary.

The package boundary of a package.json file is deemed to be all modules in that folder and subfolder until the next nested package.json file.

The plan is to add a top-level --mode flag to this, roughly matching the npm proposal, but restricted to the current package boundary. This work is currently at https://github.com/guybedford/node/tree/package-top-mode.

The mode effectively marks a directory as an "esm mode" directory, where all ".js" files in that directory are loaded as ES modules, unless they are located in a subfolder that contains a package.json without an "esm mode", which results in reverting to the transparent interop of treating ".js" as CommonJS.

In addition this PR enables caching of package.json lookups within the ES module resolver.

Presentation: https://docs.google.com/presentation/d/1xK1ana_TIxfAHX33CYVHFnJsV0If_YirLtRBBga9cv0/edit#slide=id.p

This work is based on the following prior proposal work:

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
Affected core subsystem(s)

esmodules

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 26, 2018
@guybedford guybedford requested a review from bmeck January 26, 2018 11:09
@guybedford guybedford force-pushed the package-mode branch 6 times, most recently from 862219d to cfb5d70 Compare January 26, 2018 13:53
@bmeck
Copy link
Member

bmeck commented Jan 26, 2018

I have a writeup from conversations in form of a pseudo design document.

@@ -39,14 +43,22 @@ function search(target, base) {
}
}

const extensionFormatMap = {
const extensionFormatStd = {
__proto__: null,
'.mjs': 'esm',
'.json': 'json',
'.node': 'addon',
'.js': 'commonjs'
};
Copy link
Member

@jdalton jdalton Jan 26, 2018

Choose a reason for hiding this comment

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

☝️ still not a fan of using short forms for most names but not commonjs. We have ECMAScript module -> esm, JavaScript Object Notation -> json, CommonJS -> should be cjs.

Copy link
Member

Choose a reason for hiding this comment

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

tbh I have no idea why I made this change and if it had been brought up before my PR landed I would have most likely changed it back, kinda feelin' guilty about this one.

Copy link
Member

Choose a reason for hiding this comment

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

Babel now allows specifying "cjs", in addition to "commonjs", as their module mode because folks kept reaching for it.

Copy link
Member

Choose a reason for hiding this comment

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

I would also like to see it changed back to "cjs"

src/node.cc Outdated
}
config_main_mode = arg + 7;
if (config_main_mode != "esm") {
fprintf(stderr, "%s: \"%s\" is not a valid module mode\n", argv[0],
Copy link
Member

@jdalton jdalton Jan 26, 2018

Choose a reason for hiding this comment

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

👆 I'm not a pro in c++ but is config_main_mode = arg + 7 a number result? If so would it never match "esm" in the if statement.

Copy link
Contributor

@jkrems jkrems Jan 26, 2018

Choose a reason for hiding this comment

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

config_main_mode is a std::string, so comparing using the operator works. :) I assume arg + 7 is the pointer to the null-terminated string starting 7 bytes from the pointer arg, skipping the --mode= (7 bytes). But it's not the most straight-forward to follow.

src/node.cc Outdated
args_consumed += 1;
config_main_mode = main_mode;
if (config_main_mode != "esm") {
fprintf(stderr, "%s: \"%s\" is not a valid module mode\n", argv[0],
Copy link
Member

@jdalton jdalton Jan 26, 2018

Choose a reason for hiding this comment

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

👆 If there is two modes documented shouldn't both modes be allowed?

Copy link
Member

Choose a reason for hiding this comment

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

it does seem like you should be able to set "commonjs" or "cjs" as the mode

@jdalton
Copy link
Member

jdalton commented Jan 26, 2018

The mode effectively marks a directory as an "esm mode" directory, where all ".js" files

What about extensionless bin files that CLI's often use?

Update

There's a mention in the writeup about --format related to extensionless but I'm not sure why a mode property couldn't cover it.

The writeup mentions a gotcha with require and extensionless files.
Does this mean require can load ESM?

@JacopKane
Copy link

Thanks a lot for your work that might save us from a new javascript extension.

I know this is an entirely minor detail to consider right now, but I would only argue using shorthand, esm, instead of something more descriptive like esModule, ecmaModule or maybe something else but just more apparent.

@bmeck
Copy link
Member

bmeck commented Jan 26, 2018

@jdalton --format is for entry points that don't have extensions at all / STDIN, it does not alter the default format of file extensions. If --mode affected those situations it would need to expand scope so that all extension-less files were treated in a specific way. This complication is why it was punted.

@jdalton
Copy link
Member

jdalton commented Jan 26, 2018

@bmeck

This complication is why it was punted.

What about tackling it in the shebang of the bin file like

#!/usr/bin/env node --mode esm

@bmeck
Copy link
Member

bmeck commented Jan 26, 2018

@jdalton punted and as describe in the design doc "bin" files can have extensions even if the command does not which is the common case for using command line wrappers, feel free to PR the --format flag as described if you feel strongly about needing that specific use case.

@jdalton
Copy link
Member

jdalton commented Jan 26, 2018

punted and as describe in the design doc "bin" files can have extensions even if the

The bin file is opting in to esm with the --mode in the shebang. It's not executing the command. Node would parse it out. How you handle conflicting signals is a separate issue.

@bmeck
Copy link
Member

bmeck commented Jan 26, 2018

@jdalton --mode is setting up file extension mapping similar to a MIME DB, --format is a very different idea which overrides something that doesn't have a well known intention already. We absolutely should not conflate these flags in my opinion.

@jdalton
Copy link
Member

jdalton commented Jan 26, 2018

We absolutely should not conflate these flags in my opinion.

Having it specified in the shebang is a nice descriptive and natural way to tackle extensionless bins.

@bmeck
Copy link
Member

bmeck commented Jan 26, 2018

@jdalton PR up the --format flag :)

@jdalton
Copy link
Member

jdalton commented Jan 26, 2018

Naw. It doesn't have to be --format or bust.

@jkrems
Copy link
Contributor

jkrems commented Jan 26, 2018

@jdalton One problem with putting arguments into the shebang is that it's not super portable. :( https://unix.stackexchange.com/questions/63979/shebang-line-with-usr-bin-env-command-argument-fails-on-linux

P.S.: Shipping something like a nodem or mnode shim with node might fix that though.

@jdalton
Copy link
Member

jdalton commented Jan 26, 2018

Or we could try it in this PR, write tests, and see if issues are had.

@jkrems
Copy link
Contributor

jkrems commented Jan 26, 2018

Or we could try it in a PR, write tests, and see if issues are had.

Not sure I follow - this isn't hard to verify. The following:

$ cat test.js
#!/usr/bin/env node --harmony
console.log(process.argv, process.execArgv);

Works on OSX but won't run on (all) Linux (the same without --harmony works on both):

./test.js
/usr/bin/env: node --harmony: No such file or directory

@jdalton
Copy link
Member

jdalton commented Jan 26, 2018

Not sure I follow - this isn't hard to verify. The following

Many times ideas are shot down with a quick SO post without actually seeing if, in the given scenario, it is doable or an issue is able to be worked around.

Pinging @ceejbot and @chrisdickinson who had this in their npm proposal, though it was unimplemented (wondering if they ran into similar issues or if they've thought of anything to workaround the gotcha).

P.S.: Shipping something like a nodem or mnode shim with node might fix that though.

That may be it 😋. I never considered something like that. Kinda neat!

@giltayar
Copy link
Contributor

giltayar commented Jan 26, 2018

Question: with such a proposal, how would I write a "dual mode package", one which works whether we require the package or whether we import it. This is possible in the mjs proposal, but I don't see how you can do it in this proposal.

If we go with this proposal, all library developers that want to migrate to the esm model will need to fork their modules. So we will have moment and moment-esm. lodash and lodash-esm, uuid and uuid-esm, etc...

Given that we're going to live in dual-mode world for a long time, I believe this is not viable.

@bmeck
Copy link
Member

bmeck commented Jan 26, 2018

@giltayar You would still use the .mjs approach for such a behavior. This PR is ontop of .mjs and seeks to alleviate common cases that do not need such a behavior since it is already available through .mjs. A variety of situations do not need to support dual mode packages, such as top level applications that are never meant to be treated as dependencies.

@devsnek
Copy link
Member

devsnek commented Apr 28, 2018

@BridgeAR i re-opened nodejs/modules#42 which should be good enough for now i suppose

@MylesBorins MylesBorins added the blocked PRs that are blocked by other issues or PRs. label Apr 28, 2018
@guybedford guybedford force-pushed the package-mode branch 2 times, most recently from d6d6913 to f61aa91 Compare June 14, 2018 19:12
@bmeck bmeck mentioned this pull request Jul 9, 2018
@MylesBorins MylesBorins added this to In Progress in ECMAScript Modules in Core Jul 13, 2018
@MylesBorins MylesBorins moved this from In Progress to In Review in ECMAScript Modules in Core Jul 13, 2018
This implements a package.json "mode": "esm" module boundary. When
loading a module from a package with this esm mode, ".js" files are
loaded as es modules.

In addition package.json caching is implemented in the module lookup
process, and package.json boundaries are only checked when necessary.
@ChALkeR
Copy link
Member

ChALkeR commented Mar 1, 2019

@devsnek nodejs/modules#42 is closed, any other reference?

@devsnek
Copy link
Member

devsnek commented Mar 1, 2019

@MylesBorins maybe has something, I'm not 100% sure

@richardlau
Copy link
Member

Is this superseded by #26745?

@GeoffreyBooth
Copy link
Member

Is this superseded by #26745?

Yes.

@richardlau richardlau closed this Mar 19, 2019
@MylesBorins MylesBorins moved this from In Review to Complete in ECMAScript Modules in Core Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. wip Issues and PRs that are still a work in progress.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet