ESM_FORMAT: Avoid reading package.json when extension is unambiguous

[mathiasbynens]

It’s wasteful to load the package.json even if the extension is unambiguous. This patch optimizes for the zero-config case by avoiding needless package.json loads.

With this patch, the mental model becomes a lot simpler too:

• .cjs means commonjs
• .mjs means module

See discussion thread: https://twitter.com/mathias/status/1096167102455709696

Checklist

• commit message follows commit guidelines

[ljharb]

I don’t think this is a future-looking optimization; eventually package.json should be able to override any extension to parse in any way.

[mathiasbynens]

That’s what’s under discussion. IMHO it’s worth carving out .cjs and .mjs since they’re unambiguous, and only enable overriding other extensions. From the Twitter thread it sounded like people were on board with that.

We need to weigh the cost of allowing .cjs and .mjs to be overridden (i.e. load package.json all the time) with the performance benefits of not having to do so for the zero-config case.

[robpalme]

This PR looks good.

My view is that package.json is about defining package-scoped things, whereas extensions/pragmas are a way of specialising/overriding behaviour on a per-file basis.

This locality simplifies the mental model and eliminates a data-dependency.

[guybedford]

Thanks @mathiasbynens for the PR. This is a simplification of the existing spec that seems sensible to me.

There is effectively only one behavioural change here from what was written previously and that is that previously if there was no package boundary, we would load .mjs files as CommonJS which seems wrong anyway.

This sounds like a spec bug to me, where the fix happens to enable an optimization.

We as a group can decide over time whether we want that optimization to remain, but what we're really deciding from a behavioural perspective is if node x.mjs where there is no package.json at all should load as a module.

[mathiasbynens]

what we're really deciding from a behavioural perspective is if node x.mjs where there is no package.json at all should load as a module.

+1 FWIW, there is precedent for this in both jsc and d8. It seems reasonable, and even expected, to me.

[guybedford]

Note that the implementation already does this too, so that this is actually just a spec bug.

[GeoffreyBooth]

This is fine for where the implementation is now, but if we add package exports we'll always need to read package.json to know what the valid paths inside a package are. Also package-level loaders, if added, would also require always reading package.json. Ditto for file extension configuration like @ljharb said.

Another point is if type: module ever defines anything in a package more than just file extension mappings, separately than what .mjs defines (globals like Buffer come to mind, and the discussion about making them package-scoped).

But for right now, sure.

[guybedford]

Just noticed this line is missing.

[MylesBorins]

Lgtm

[ljharb]

We don’t even have consensus that the cjs extension will be a thing, and no extension should be special - if any of them can’t br overridden, then none of them can (including js)

[guybedford]

@ljharb please read #33 (comment) and then let me know if you want to revise your comment.

[ljharb]

@guybedford that comment hasn’t made anything clear to me about why I’d revise it.

In the absence of a package.json, and perhaps when run as an entry point, it makes sense that a file extension unambiguously determines the parse goal, but if a package.json is present (and certainly when it’s not an entry point), it must always be consulted regardless of the file extension. No extension should be special; either they can all be overridden from package.json or none of them can.

Separately, adding the cjs extension, without any consensus besides apparent side channel discussions about it, isn’t something that should be done via a PR.

[guybedford]

In the absence of a package.json, and perhaps when run as an entry point, it makes sense that a file extension unambiguously determines the parse goal

Currently, by the spec, node x.mjs where there is no package.json will execute as CommonJS, so this is not the case as written.

The change here ONLY changes the above. That is the only behavioural change.

[Trott]

I imagine this can/should be closed at this point? Or if not closed, then at least transferred to the main repository?