nodejs / node-eps

Node.js Enhancement Proposals for discussion on future API additions/changes to Node core
442 stars 66 forks source link

adjusted proposal: ES module "esm": true package.json flag #60

Closed guybedford closed 6 years ago

guybedford commented 7 years ago

Update: This proposal has been updated to reflect the direction I believe to be the simplest and best for the ecosystem - an "esm": true flag in the package.json file.

The reason for this change from module is due to the interactions of module with other conditional main systems resulting in much unwanted complexity. For example browserModule was the natural next step here, which I don't think is a road that would be good for the ecosystem at all.

(added with comment at https://github.com/nodejs/node-eps/pull/60#issuecomment-343136052)

This spec provides a package.json "module" property and resolver implementation approach, to allow distinguishing ES modules in NodeJS. Unlike the In Defense of Dot JS proposal, it excludes supporting the "modules" and "modules.root" properties, while providing alternatives to the workflows these were designed for.

I've found in my own tooling work that having an active resolver spec would help a lot to ensure convergence towards where NodeJS will ultimately be heading. Webpack and Rollup both already implement this package.json "module" property, but the edge cases are not completely clear. If we have a formal active spec for "module", hopefully we can start to get everyone on board with a future-facing solution here.

seishun commented 6 years ago

The appeal of this proposal for me was that it provided a path towards a future where Modules are the default rather than opt-in. But an "esm": true property isn't any better than .mjs in that regard. In fact, requiring everyone to add a boilerplate line to their package.json seems worse than an extra letter in the extension.

guybedford commented 6 years ago

@seishun I understand that, but tools that generate package.json files such as npm could auto-include this taking the cognitive overhead away from the user (jspm will add it by default). Using a flag to upgrade the ecosystem really is the standard approach in a situation like this without backwards-compatible alternatives. module is a nice idea, but as mentioned, I find the edge cases just ruin it from being practical (I only mentioned browser field interaction, but there is also bin, react-native with module etc etc to think about that become painful). module needs a flag mode anyway to handle anything except setting the main, so it's basically just a flag with sugar. Also consider Q:"why is it saying invalid token 'export" A:"Oh, you didn't add the module main"... which is a complete conflation of concerns! Much better to have "Oh, you need to set the module mode flag, npm will auto-generate this for you usually" I think - less cognitive overhead in total. Of course ".mjs" is the simplest approach as discussed, but I think the esm flag is the simplest approach for keeping ".js".

tilgovi commented 6 years ago

Unfortunately, this seems to mean that one cannot publish a package that has both types of entry point. Maybe that's even for the best, but it's worth pointing out.

guybedford commented 6 years ago

@tilgovi that would be possible by including a nested package.json file without the property. This use case is discussed here - https://github.com/nodejs/node-eps/pull/60/files#diff-3cc8b0491eaf7227ad6756b1d85d3f65R89.

guybedford commented 6 years ago

(and of course .mjs is a good way to use both as well)

rektide commented 6 years ago

The use of "esm": true in package.json to signify a ES module is a really good strategy. I will be including the new "esm": true. I hope tools move to "esm": true. I already publish pure ES modules to npm, and I want a way to indicate this, and it sends a clear consistent message to all tooling what I have: ES modules. I support the new proposal, which replaces "modules": "[path]" with "esm": true.

There are however really good tools such as rollup which are based around the historical module directive. Since I am distributing ES npm modules, I feel that at the current time it makes sense for me to include the module directive in package.json, as a means of supporting "legacy" tooling.

So until "esm" support is more pervasive (which I hope is soon), I'll be publishing my modules like this:

{
    "name": "my-package",
    "esm": true,
    "module": "./my-package.js",
    "main": "./my-package.js",
}
jamiebuilds commented 6 years ago

Can we consider alternate names other than "esm"? I've seen it be the source of confusion a number of times, where "module" was fairly well understood. Could "modules": true work?

guybedford commented 6 years ago

@thejameskyle certainly I'd be open to another name, and I'd be happy to go with whatever name can get the consensus as well. I get your point about choosing something intuitive to a first time user. The only thing is ensuring modules has no other meaning elsewhere. Perhaps we need to be very clear as well that it should be equal to true and not just truthy to avoid any possible conflation.

guybedford commented 6 years ago

The other worry with modules is users confusing it with module, since that already has meaning.

guybedford commented 6 years ago

My preferences for esm are that it is very unique (not currently used in package.json), and because it is just three letters it is easy to type and become invisible like a doctype does. For example, few of us have any clue what doctype really means but we're happy to write it as an early learning in web dev.

hax commented 6 years ago

IMO, doctype is easy to understand as the abbr of document type, but it's not easy for novices to recognize esm as the abbr of ecmascript module.

Anyway, I think esm is ok for current usage. But I also hope we can get a more friendly name when it being adopted widely.

guybedford commented 6 years ago

@hax it is important to go with a name that can stick and that can get consensus, so thanks for the feedback. Perhaps "modules" does make more sense.

I've put a vote out on Twitter - https://twitter.com/guybedford/status/935432285423271936. I'd be happy to move forward with any clear winner :)

matthewp commented 6 years ago

I don't think anyone has properly responded to @mcollina's comment here: https://github.com/nodejs/node-eps/pull/60#issuecomment-317181383

Is it true that under this proposal running node index.js would first search for a package.json in order to see if this flag is set?

guybedford commented 6 years ago

@matthewp yes exactly, although note that (a) this lookup (including misses) is cached for the application lifetime, (b) search to root at worst case is fast and (c) many projects have a package.json so will exit this loop early anyway.

matthewp commented 6 years ago

We should be removing extraneous checks that degrade performance, Node already has too many of them, not adding more. Especially when the only value added is not wanting to use a different file extension. Much better, I think, to add some sort of --esm flag as an opt-in.

guybedford commented 6 years ago

@matthewp it's important to keep the statting down certainly, but if that is your concern, do you inline extensions into your module requires (this would save two stats per module load!)? We're talking of the order of two stats here anyway, so it is the same difference of one inlined extension over a codebase of hundreds of modules! Important to consider these numbers in perspective.

Certainly any work here should be carefully benchmarked.

If you believe strongly in an alternative approach, please do propose something though.

guybedford commented 6 years ago

@thejameskyle the poll results show ~55% for "esm" and ~37% for "modules" and ~7% for neither (over 774 votes, with 1% in these numbers lost to rounding). Would this be enough for you as some consensus here?

jamiebuilds commented 6 years ago

Well, the results are inherently scewed towards people that already know what "esm" means... which isn't the crowd of people I'm concerned about. Although I'm honestly surprised people who do know what "esm" means actually want that as the property.

matthewp commented 6 years ago

@guybedford Yes, I do include the extension in my require calls. Whether someone does that or not is up to them and only affects their code.

The troublesome thing about this proposal is that it slows down all uses of Node, whether this feature is being taken advantage of or not. A hello world Node script will now traverse directories searching for a package.json, parse it if it finds one, and then do what you wanted it to do.

guybedford commented 6 years ago

@matthewp yes Node will resolve modules, including loading package.json files for node_modules packages before executing your code, I'm not sure what about that is surprising.

matthewp commented 6 years ago

I think you're misunderstanding me, if the program is:

hello.js

console.log("Hello world!");
> node hello.js

My understanding is that, today, this will open hello.js (and only hello.js) and execute it as a CommonJS module.

And under your proposal it will search for a package.json, parse the package.json (if it finds one), and then open hello.js.

Is this inaccurate in either case? Node doesn't, today, start resolving modules until there is a require() call, right?

guybedford commented 6 years ago

What I don't understand is why you're focusing on a simple case, when the majority of NodeJS applications have dependencies. I'm simply trying to shift the argument back to the important case of a larger application with dependencies, where dependency resolution is the primary resolver overhead, and noting that your case is completely negligible in comparison.

matthewp commented 6 years ago

I'm focused on this case because Node's resolver is already a problem and I don't want it to become worse. It's important to remember that Node.js is used in a variety of contexts and not just for large server applications. I've personally been forced, in the past, to bundle a CLI program because the resolver was too slow. Executing scripts in a loop is a real thing!

As far as I'm aware, at least today all slow paths are opt-in (module identifiers that point to directories, using ./package to load a JSON file, etc), and this is the first one that explicitly punishes everyone, and for a purely superficial benefit that only some care about.

guybedford commented 6 years ago

@matthewp yes as the proposal stands that extra package.json load check would not be opt-out. Thinking about this, an --esm flag could possibly be added here to override treating the entry point as being at an ES modules directory boundary, overriding the package.json check. Would such an addition work for you here?

matthewp commented 6 years ago

I'm pretty uneasy with any unwanted side-effect file opening. I would certainly hope that node main.mjs (note it's mjs extension) would not have to search + open + parse a package.json file.

I'll refrain from commenting further to avoid being too negative about this idea. What I really hope that the Node maintainers consider is that these sorts of sugar proposals, while nice in some ways, can wait until after the initial release of ES modules. I hope that it is initially released only supporting .mjs and give some time to bake and get feedback before accepting any sugar proposals.

jdalton commented 6 years ago

FWIW @std/esm supports flag use,node -r @std/esm, and .esmrc or package.json or ESM_OPTIONS env configs. Users :heart: it. Each has their use and solves a different scenario.

Perf isn't an issue since the lookup is done once per directory not per module in the directory. I test with lodash-es which has ~644 modules.

yawetse commented 6 years ago

was there any resolution to using .m.js instead of .mjs?

guybedford commented 6 years ago

@jdalton yes this proposal is similarly done once per directory and cached for the lifecycle of the application. Glad to hear it hasn't been a perf issue for your users.

Trott commented 6 years ago

I imagine this conversation, if still ongoing, is happening elsewhere. Please comment or re-open though if closing this isn't the right move.

demurgos commented 6 years ago

For reference, this topic seems no longer discussed but most of the discussions around modules moved to the nodejs/modules repo.