CJS named exports via two-phase execution

[guybedford]

This implements the alternative to Dynamic Modules, supporting CommonJS named exports through a two-phase execution. I understand that some members of the group will be against this approach due to the different execution invariant as described below, so we will likely need a vote on this. @bmeck @ljharb consider your disapproval to already be known :)

With this approach, we can think of CommonJS modules as being "precached" in the CJS loader at instantiation time, which allows us to know the named exports.

The two-phase execution guarantee we provide for this, is the following:

At the end of instantiation, all CJS modules will be executed synchronously in their post-order as referenced by the ES module graph. Next, we synchronously execute all ES modules in the graph in post-order.

This means we retain the execution invariant that import 'cjs1'; import 'cjs2' will always execute cjs1 before cjs2, and execute them synchronously in order.

The execution invariant lost here is that import 'cjs1'; import 'esm'; import 'cjs2' will execute cjs1, cjs2 in sync order, followed by esm in the main execution phase.

The benefit of this approach is we get named exports for CJS! In addition this approach supports export * from 'cjs' as well, solving one of the concerns of the dynamic modules specification.

Most of the work in the PR here is ensuring the careful CJS sync execution phase.

[ljharb]

As much as i want fully transparent interop, to make the block explicit: this would make “node.js” an inaccurate name, since it would violate the JS spec.

[devsnek]

same reason as ljharb

[guybedford]

As much as i want fully transparent interop, to make the block explicit: this would make “node.js” an inaccurate name, since it would violate the JS spec.

Jordan, this isn't true. As mentioned we are aware of your position and to reiterate the counter-argument here, if you consider the module record for CommonJS as a shell module representing an ES module view into a CJS module, then the ECMAScript specification has absolutely no bearing on when the underlying CommonJS execution must happen, as it doesn't have any concept of CommonJS.

Dynamic Modules were our attempt to integrate CommonJS modules directly on top of the ECMAScript specification, which failed. As such this is the only alternative for named exports.

[ljharb]

First of all, I don’t think it has fully failed yet - there are far fewer objections to dynamic modules than there already are to this approach, so if we’re going to put anything to a vote, it’d be dynamic modules.

As for this case, my understanding is that the entire module graph needs to be linked before any user code can be evaluated.

Either way, if this is the only other alternative, then to me that means there is no alternative.

[guybedford]

First of all, I don’t think it has fully failed yet - there are far fewer objections to dynamic modules than there already are to this approach, so if we’re going to put anything to a vote, it’d be dynamic modules.

The topic of Dynamic Modules has now been brought to TC39 over 7 times, 5 times for the current dynamic modules specification. In the last meeting there were three separate technical arguments made by four different members as well as the author of the ECMAScript modules specification, against dynamic modules from TC39, all of which are irresolvable without significant refactoring of the entire ECMAScript modules specification.

so if we’re going to put anything to a vote, it’d be dynamic modules.

We don't have an ability to vote on dynamic modules, when TC39 has not given it to us. It's simply not on our timeline anymore.

As for this case, my understanding is that the entire module graph needs to be linked before any user code can be evaluated.

JavaScript is an asynchronous environment, and linking is asynchronous. User code running during linking is not only spec-compliant but is the typical scenario for any application due to the asynchronous nature.

Either way, if this is the only other alternative, then to me that means there is no alternative.

Certainly, I appreciate your opinion here completely. We definitely will need a vote to resolve this one, and I sincerely hope we can give users the experience they expect. If not, I accept that too.

[GeoffreyBooth]

this would make “node.js” an inaccurate name, since it would violate the JS spec.

As far as I’m aware, the ECMAScript spec doesn’t include wrapping code in (function (exports, require, module, __filename, __dirname) { ... }) blocks. So by this standard Node.js is already an inaccurate name, since Node includes CommonJS—and it’s enabled by default, not opt-in.

And before you say “but that’s legacy, from before ES modules were designed!” CommonJS is still what we’re discussing here. This is how to make CommonJS work with ESM.

Making Node truly spec compliant—dropping CommonJS—isn’t on the table. If people want that, they’ll use another runtime. So the question really is just now much deviance from the spec we’re willing to allow.

[ljharb]

The spec does not preclude such a wrapper - it just means CJS isn’t strictly a Script. It does currently preclude any evaluation before all linking is completed, as far as i understand it.

[jdalton]

@guybedford

Just double checking: Does this two-phase execution proposal require changes to ECMA262?

[guybedford]

@jdalton not at all, the CJS phase is a userland phase, just like any other loader could have its own logic.

[devsnek]

note that i tried to add this a while back: nodejs/node#16675

and more recently in my own experiments which come a bit closer to this: https://github.com/devsnek/esm_loader/blob/master/src/index.js

however i still don't think we should do this in core - at least by default. this is very brittle and given the culture of singletons and side effects in cjs it would be very surprising to me if my db got corrupted during the instantiation phase of esm.

[GeoffreyBooth]

So . . .

As much as i want fully transparent interop, to make the block explicit: this would make “node.js” an inaccurate name, since it would violate the JS spec.

and

Just double checking: Does this two-phase execution proposal require changes to ECMA262?

not at all, the CJS phase is a userland phase, just like any other loader could have its own logic.

seem to me to not be in agreement. So does this PR violate spec or not? If it does, how is it noncompliant? If it is noncompliant, is it noncompliant in any way that CommonJS in general isn’t?

[ljharb]

Loaders would have to run in their own realm - modifications to globals (or global variables) couldn't survive to be observable by ES Module code. That's not the same thing unless someone's proposing running CJS modules in a different realm than ES Module code (which would break any usage of instanceof on builtins).

[GeoffreyBooth]

Loaders would have to run in their own realm - modifications to globals (or global variables) couldn’t survive to be observable by ES Module code. That’s not the same thing unless someone’s proposing running CJS modules in a different realm than ES Module code (which would break any usage of instanceof on builtins).

I don’t see how this answers my questions. Supporting (or not) CommonJS modifications of globals, or supporting instanceof on builtins, would seem to be separate concerns than spec compliance.

[ljharb]

@GeoffreyBooth sorry, let me clarify. "Loaders" aren't something really covered by the spec; that a loader can be written in JS (as an ES Module or as a Script) doesn't change that, as far as actual ES Module code is concerned, it has to be as if the loader never ran any code - in other words, loaders are a kind of magical thing that run in their own universe, set up node to load modules, and then vanish, leaving no other observable trace.

However, if an ES Module does import 'esm'; import 'cjs'; console.log(typeof Array.prototype.map);, and esm contains console.log(typeof Array.prototype.map) and cjs contains delete Array.prototype.map, anything that causes those two console logs to be anything besides 'function' and then 'undefined' is violating the spec (because code has run in an improper order).

It's possible the spec doesn't contain precise language that explicitly forbids this, but when it's been brought up in the past by @bmeck, the entire committee's opinion, as far as I can recall, was "we aren't sure how to write the spec text to forbid it, but it is absolutely forbidden, and we will write whatever text is necessary to prevent it should the scenario arise".

[GeoffreyBooth]

So

The spec does not preclude such a wrapper - it just means CJS isn’t strictly a Script. It does currently preclude any evaluation before all linking is completed, as far as i understand it.

sounds to me a lot like “the spec technically doesn’t say that we can’t do this, so we will.” Whereas

It’s possible the spec doesn’t contain precise language that explicitly forbids this, but … the entire committee’s opinion, as far as I can recall, was “we aren’t sure how to write the spec text to forbid it, but it is absolutely forbidden, and we will write whatever text is necessary to prevent it should the scenario arise”.

sounds to me a lot like “the spec technically doesn’t say that we can’t do this, but we shouldn’t!”

It seems to me that if we’re going to take advantage of the spec’s gaps in the former case, there’s nothing stopping us from continuing to do so in the latter case; indeed, it would only be consistent.

So @ljharb and @devsnek unless you can point to something specific in the spec that this PR is violating—other than your read on the committee’s feelings—I encourage you to drop your objections, at least on that basis. I understand if you want to object on other, very reasonable concerns, but let’s not claim the spec says something that it doesn’t, unless you can prove it. “This would make ‘node.js’ an inaccurate name” comes across to me as snootily dismissive of @guybedford’s work here, and I think he deserves a more careful explanation of what spec problems you think that this PR has if indeed you think it has any.

As for the unexpected results because of load order concern, sure, that’s a reasonable concern. But it would seem to me a separate issue, and one more of UX than spec compliance. If we go this route, we would have to educate users on what it meant to import both CommonJS and ESM dependencies into the same file. That’s valid, but not necessarily a blocker. The committee might very well decide that that’s a better option than no CommonJS import interoperability at all, or the dynamic modules approach that has its own issues.

[ljharb]

@GeoffreyBooth what's stopping us is that the instant we decide to do it, the spec will add something prohibiting it - that's not the case for CJS. If you won't be convinced by TC39 members telling you that, then certainly we can change the spec - but I guarantee you that we will, it will be forbidden, and node will be violating it.

imo changing the entire semantics of the ESM design to alter execution order isn't just something we can "teach" users, it erases the benefit of shipping ESM at all.

[ljharb]

To be more concrete, nodejs/node#16675 is the node PR where this was attempted, and here's the TC39 notes discussing this: https://github.com/tc39/tc39-notes/blob/master/es8/2017-11/nov-28.md#conclusionresolution-13

[SMotaal]

@guybedford Can you (sorry if being redundant) confirm if this would:

1. Fully address implicit esm depends on cjs, and,
2. not break explicit cjs depends on cjs (specifically require), and,
3. still allow explicit cjs depends on esm (specifically import(…)).

So what I am getting at here is if partial interop will be more likely the reality for moving forward in that direction, right?

[Trott]

I imagine at this point this can/should be closed?