Closed Fishrock123 closed 7 years ago
I would say that we care about all of these and there's no particular priority ordering for them. The single most important issue is that ESM fundamentally breaks with the way Node.js currently handles modules; and the differences are such that they cannot be easily reconciled or glossed over. Each of the points you list above are merely examples of that point.
The summarization of these points is straightforward: We cannot implement ESM in core without fundamentally breaking one or more existing aspects of how Node.js currently operates. Given that we prioritize ecosystem stability over language support, the end result is that we really cannot implement ESM in it's current state and therefore are requesting that TC-39 agree to work with us on revising the ESM specification. If TC-39 is not willing to revise ESM, then I see no choice but for us to sacrifice on spec compliance by not fully supporting the ESM specification as currently defined -- we might be able to get part of the way there, but we would need to identify what subset of capabilities we are able to support. If TC-39 is willing to revise ESM, then our task is to come up with the list of specific changes that we would like to see in order to make implementation feasible, spend some time working up the justification for those changes, and get those in front of the committee for consideration.
Of course, that said, there may be a path forward here that we simply are not seeing yet. Hopefully, someone on TC-39 will have some ideas there that we can kick around -- I am cautiously optimistic on that front but not overly so.
Module immutability definitely is a blocker for my uses. Import and loader semantics differences I think can be "solved" to some extent by just supporting ESM as a flag where you can only have one module system or the other at runtime, but never both. A bit unfriendly, but the only reasonable option I see in the current design. Peaceful coexistence is not really possible unless the semantics match, which hasn't been well received so far...
Hi all,
Let me just respond to some of the points in the OP with my impression of the feasibility of them. Most of them seem to not be problems with the standard, and something Node.js could overcome with some edge-case caveats and support from V8. So in my opinion many of them are issues to bring up with V8.
I want to be sure that, for whatever subset of these goals you end up deciding to push for, you bring your concerns to the right place. If the impression is currently that these are problems with the standard, then I think things might end up over-confrontational, as Node's concerned might be "dismissed" because they're not a problem for ES.
I apologize if people are already aware of what I say below; I think Bradley in particular is pretty deep in this space so what I say here might be redundant to him.
(1) Import disparity between NCJS and ESM
This is hackable with some effort on your side, assuming V8 gives you even the most basic of hooks. You will get a request that looks something like (moduleSpecifierString, referringModuleDataStructure) and be told to produce a new module data structure (a "module record" in the spec; V8 will presumably have a C++ version). Given that you can find the NCJS exports object with these inputs, it's very easy to have your ES exports data structure be the simple "default" => NCJSexportsObject. But you should be able to loop over the Object.keys or Object.getOwnPropertyNames or even proto-climb the NCJS exports object, and create additional keys in the ES exports data structure.
This could be done fully generally, or it could be special-cased for core modules. If done fully-generally, it would have edge cases around conditional exports; the method I describe would take a snapshot of the exports at the time the first import happens.
(2) Async Loading
What you've written here sounds right. However, I think the VM will give you hooks to do fully synchronous loading, and so if you bring this to TC39, some of them they may not find this concern compelling.
My understanding is that you want to be web-compatible and future-compatible with anything that makes use of async loading (e.g. the always-controversial top-level await
). This is a great goal and one I support 100%, so I am sympathetic to this concern, and know that other more realistic members of TC39 are too. But there are some people on TC39 who will be like "Web? What's that? Not our problem; we already gave you the hooks to do whatever. We just write a host-environment-agnostic spec!" It's something to watch out for.
(3) Conditional Imports / Exports
As you said, on the imports side this is being addressed, and not really on the exports side. (I doubt there will be much sympathy on the exports side; I think the committee's general view is that a module's shape must be static, and if not, just use a default export which is mutable over time. But if this is something you decide is important, it's worth bringing up.)
(4) Mocking Tooling for Testing
While existing tooling won't be drop-in possible due to the new APIs, it should be totally possible to create new tools based on new APIs that Node exposes, possibly with some help from V8 to ensure they give you all the flexibility the spec allows.
Assuming V8 allows, you should get total control, per the (moduleSpecifierString, referringModuleDataStructure) -> newModuleDataStructure hook I mentioned. Since you have control over this, you can expose Node-specific APIs that allow user code to intercede and do whatever it wants during that hook. Hopefully you can see how this is powerful enough to allow anything.
The only invariant you need to preserve per-spec is that if module A does import './b.js'
, then the next time it does import './b.js'
in the same file, it returns the same object. (In other words, the above hook must be idempotent, for the exact same input arguments.) This seems like a reasonable limitation; I've never seen a mocking tool that wants to violate it. If you do, then specific cases in the Node ecosystem where that's been valuable would be good.
(5) Injecting inspection hooks.
Similarly to (4), the spec has a fully-general hook; you can use expose them to users. There shouldn't be any spec change required here.
(6) Overloading other modules.
I think you could use the hooks mentioned in 1, 4, and 5 to allow this. However, it would have to be the case that graceful-fs is imported and executes before anyone else imports fs, due to the fact that you can't override a module's exports. (You can just replace it wholesale via those hooks, for all future importers.)
This is kind of already true with require
, I think? In that if someone does const { readFile } = require("fs")
, and then later does require("graceful-fs")
, readFile
won't have the new behavior. Right?
I understand this area worse than the others so apologies if I completely missed the requirements.
REPLs
Heh. REPLs are completely unspecified and somewhat of a free-for-all. What a "program" means in a REPL is usually a single line of code entry. At least that's what it means in Chrome's REPL. I think given all the fun tricks Node is already doing with vm and so on in the REPL, you'll be able to achieve any desired behavior here, and there is no spec restriction stopping you.
I'm going to go out and just state that after lots of time, I still personally am on the clean break approach. No existing code would cease to function unless a dependency introduces a breaking change by moving to ESM. Even if it is hard to swallow, it is spec compliant today. With that in mind:
topic | +/- |
---|---|
Import disparity between NCJS and ESM | -1 |
Async Loading | 0 (-1 on spec of top level await) |
Conditional Imports / Exports | +1 (see import function) |
Mocking Tooling for Testing | +1 |
Injecting inspection hooks. | +1 |
Overloading other modules. | 0 |
REPLs | 0 |
replies to @domenic:
But you should be able to loop over the Object.keys or Object.getOwnPropertyNames or even proto-climb the NCJS exports object, and create additional keys in the ES exports data structure.
The edge cases of this have been gone over several times and:
module.exports
value you get some weird sync errors where default is live, but keys are not. Due to spec constraints we also need this list of keys prior to linking which results in abusing ModuleDeclarationInstanciation to recursively evaluate during HostResolveImportedModule for both NCJS and ESM dependencies.toString
everywhere.Heavily on the side that doing suck plucking is not a good path.
However, I think the VM will give you hooks to do fully synchronous loading, and so if you bring this to TC39, some of them they may not find this concern compelling.
In order to do ESM with the current spec, we must do it async; in particular ordering differences cause sync behavior to need to do things that are not-spec compliant (including the same ModuleDeclarationInstanciation recursion as above).
This is kind of already true with require, I think? In that if someone does const { readFile } = require("fs"), and then later does require("graceful-fs"), readFile won't have the new behavior. Right?
Correct.
I understand this area worse than the others so apologies if I completely missed the requirements.
The problem is with order of evaluation and changing behavior. NCJS loads and evaluates top down, but placing require
at the top of your file is the norm. People are able to control loading graceful-fs
and patch the loader in the same dep-graph while loading is still occuring. For ESM all modules are linked prior to any evaluation of graceful-fs
leading to a need for hooks to be able to hoist themselves to evaluate prior to the current dep-graph
in order to setup things like you have mentioned. This also affects meow which has significant usage.
In order to do ESM with the current spec, we must do it async; in particular ordering differences cause sync behavior to need to do things that are not-spec compliant (including the same ModuleDeclarationInstanciation recursion as above).
Maybe we should take this off-thread, but I don't understand this. (And I don't expect other TC39ers will either.) You would just define HostResolveImportedModule to do sync filesystem access and parsing. Maybe this is "abusing ModuleDeclarationInstanciation to recursively evaluate during HostResolveImportedModule for both NCJS and ESM dependencies", but that seems spec-compliant and probably in-line with what was envisioned for HostResolveImportedModule when it was created.
I guess this also touches on "or ESM all modules are linked prior to any evaluation". Maybe there is some assert in the spec that I missed, but I think you can interleave linking and evaluation per spec. It's not great because of the browser-compat issues and so on, but I think it's technically allowed, so the aforementioned unrealistic portion of TC39 will probably bring that up.
@bmeck what does +/- on your chart represent?
+1 - is something to argue the need for / something I think can be made a path for 0 - neutral in some fashion / think path forward is fine -1 - is something to concede / don't see a path forward
@domenic
Nothing explicitly forbids it, but parts of spec / semantics require async behavior in order to function as expected:
Maybe there is some assert in the spec that I missed, but I think you can interleave linking and evaluation per spec.
I'm still getting my head around this and definitely have a long way to go... but Line item #2 here seems to rule this out. Specifically, in ModuleEvaluation()
, Line 2 says, "Assert: ModuleDeclarationInstantiation has already been invoked on module and successfully completed."
@bmecks point about 15.2.1.16.4.8.a also seems to imply that interleaving linking and eval is problematic.
(Please let me know if I'm reading this wrong however)
@bmeck I'm on the same boat as @domenic on this one, there is not such thing as "async" enforcement in the spec today, and neither a strict order of evaluation.
As for "15.2.1.16.4.8.a ModuleDeclarationInstanciation"
that you mentioned above, that is just an assertion to guarantee that the following step a. Let requiredModule be ? HostResolveImportedModule(module, required).
always return a module record. That does not imply evaluation, just the creation of the environment record for all required modules.
I'm still getting my head around this and definitely have a long way to go... but Line item #2 here seems to rule this out. Specifically, in ModuleEvaluation(), Line 2 says, "Assert: ModuleDeclarationInstantiation has already been invoked on module and successfully completed."
@jasnell that line just assert that a module can't be evaluated if it doesn't have an associated environment record, nothing else. Maybe the name of that abstract operation is confusing, but ModuleDeclarationInstantiation
is only responsible for creating the environment record with the corresponding bindings to other environment records (for each imported binding) and local bindings for hoistable declarations, that's all.
Very helpful clarification @caridy ... thank you. So, if I'm understanding correctly then, looking strictly at the ES6 spec for this, we can do the module loading completely sync by interleaving the ModuleEvaluation()
and ModuleDeclarationInstantiation( )
methods (that is, run ModuleEvaluation()
and do ModuleDeclarationInstantiation( )
on demand in the process. Is that correct?
Assuming that is correct, we still have the issue of bottom-up evaluation vs. top-down evaluation as currently implemented, and I believe there would still be concerns around circular references but I can't yet say for sure...
@caridy wouldn't resolving and evaluating synchronously cause issues with top-level await
? What sort of leeway do we have around this, considering that is not yet in the spec?
Maybe I should provide an analogy here to make sure that we are all on the same page. In node terms, ModuleDeclarationInstantiation
is equivalent to create a new vm
, holding its global object, and adding a bunch of getters to it, where each of those getters is going to point to a property from another global object associated to another vm, or will hold some hoistable declaration (from the to-be-evaluated module that was already parsed). As this point, you end up with a bunch of vm objects, one global object per vm, and a bunch of interconnected getters.
Whether the evaluation on each of those dependencies a) has ran to completion, or b) has not happened yet due to circular dependencies, or c) has been suspended (because of a top level await if we ever implemented),
is not really relevant, it only tells you whether or not you might be in a TDZ if the to-be-evaluated module is trying to use one of its getters to access a value from another vm that hasn't evaluate its module to completion yet, but the system can still work.
As for the elephant in the room (the top level await in modules, we should keep the door open, but we really need to have more discussions before at TC39 level). With the new import()
proposal from @domenic plus our proposal for nested imports, the top level await might not be used after all.
I note that:
import
NCJS, & instead provide ESM a means to require(ncjs)
,require(esm)
,import()
function is standardized,We sidestep many of the concerns about timing & asynchronicity, leaving the problems of entry point authorship intent detection and inspection / mocking hooks.
Is this a level of interoperability that we would be willing to accept? I bring this up because we've listed concerns about the spec as it stands but without an end goal for those concerns to serve. Is it imperative that we be able to import { foo } from "ncjs"
? Is import foo from "ncjs"
sufficient?
@jasnell the only two invariants here for module A are:
ModuleDeclarationInstantiation
has to happen for all its dependencies.ModuleDeclarationInstantiation
has to happen before its own ModuleEvaluation
.In the browsers, we will probably instantiate all module records in the graph before stepping into the evaluation (@domenic probably knows better), but in node, we can instantiate and evaluate at will.
@Fishrock123 and @bmeck ... working through all this, it's likely beneficial to classify these into Spec issues vs. Implementation issues. Here's a first attempt:
Issue | Description | Problem Origin |
---|---|---|
Static Exports | Exports in ESM are fixed prior to execution, Exports in NCJS are dynamic and determined at execution. Why is this an issue? Named imports in ESM are resolved and linked before evaluation; in NCJS, however, there's nothing to resolve or link until or immediately following evaluation, making support for named imports impossible. A non-trivial amount of userland code relies on this mechanism. A second issue caused by this is the complete lack of support for conditional exports (exporting or not-exporting symbols based on evaluation). | Spec |
Evaluation Order | NCJS evaluates from the top-down, ESM evaluates from the bottom up. Why is this an issue: The change in evaluation direction violates an existing invariant of Node.js modules. For instance, given the case var a = require('c'); a.foo = 1; var b = require('d'); where module 'd' also requires 'c' , there is an implicit guarantee that the foo property of module 'c' will be set to value 1 before require('d') is evaluated. |
Spec |
Immutability/Idempotency of Modules | Once an ESM is linked and evaluated, it becomes immutable, whereas NCJS modules remain mutable. Why is this an issue? A non-trivial amount of userland code depends on the ability to modify modules either during or after evaluation. One such example is graceful-fs which re-evaluates the Node.js core fs module. |
Spec |
Async Loading | NCJS modules are always loaded and evaluated synchronously. There appears to be some confusion on our part about whether ESM will require async loading in practice. If async loading is a requirement in practice, then we would violate an existing invariant assumption of Node.js that is depended on by a non-trivial amount of userland code. | Impl? |
Mocking / Inspection | Because NCJS modules are both mutable and defined at evaluation, the ability to inject mocks or inspection instrumentation is trivial. A non-trivial amount of userland code depends on this ability. With ESM, it is not clear how and where such hooks exist. There does not appear to be any explicit support or non-support for such hooks in the ESM spec. This may just be an implementation problem. | Impl? |
REPLs | The various async loading, static exports, and immutability concerns detailed above would seem to have a significant impact on existing invariants and assumptions made about the Node.js REPL. In particular, ESM's insistence on immutability is of particular concern but I suspect this is largely an implementation issue. | Impl |
Now... _assuming_ that this table is even remotely accurate, and assuming (as has been asserted in this thread) that async loading is _not_ a requirement and that Node.js can choose to use synchronous loading for ESMs, that leaves us with the first three Spec issues in the table, all of which are very closely related to one another. Perhaps, then, that is where we should start pushing.
@domenic, with regards to the static exports issue (specifically the issue around named imports not working with NCJS modules), you said:
This is hackable with some effort on your side, assuming V8 gives you even the most basic of hooks. You will get a request that looks something like (moduleSpecifierString, referringModuleDataStructure) and be told to produce a new module data structure (a "module record" in the spec; V8 will presumably have a C++ version). Given that you can find the NCJS exports object with these inputs, it's very easy to have your ES exports data structure be the simple "default" => NCJSexportsObject. But you should be able to loop over the Object.keys or Object.getOwnPropertyNames or even proto-climb the NCJS exports object, and create additional keys in the ES exports data structure.
I'm fairly certain I understand what you're saying here but the question that sticks out is when those additional keys in the ES exports data structure would be created. If the additional entries are added during evaluation, wouldn't we still end up with an issue with named imports given that the imports are resolved prior to evaluation? Or am I simply missing something fundamental here (which is quite likely)
You also said:
just use a default export which is mutable over time
This does provide a possible path forward for at least part of these issues. One could easily imagine a case such as import v from "mod"
where v
is the mutable module.exports
object from NCJS. This, of course, gets us back to the issue with named imports not working consistently with NCJS but that might be an acceptable tradeoff. Rather than import {statSync} from "fs"
, we'd have import fs from "fs"; fs.statSync(...);
, or some such. It's ugly from the point of view of supporting ESM is concerned but it's at least consistent with what Node.js developers currently do with const fs = require('fs')
. That's likely acceptable.
That doesn't address the order of evaluation issues, however. The difference between top-down evaluation vs. bottom-up are fairly significant and are not something that I've seen addressed yet.
Now, I'm quite new to looking at this problem space and it could very well be that everything I'm saying here is utter nonsense. If that's the case, feel free to point it out ;-)
@chrisdickinson imo it's imperative that require('esm')
and import foo from 'cjs'
work without having to know the format of the module you're importing - whether import { foo } from 'cjs'
can or must work is a separate question (although it's ideal if it does).
@jasnell I am very confused at this point, I don't understand why are we talking about "Static Exports", "Evaluation Order", and "Immutability/Idempotency of Modules" (the first 3 rows from your table).
Specifically, how is that those things will affect exiting code? If you have a NCJS today, being able to consume that from ESM is important, but those rules applied to ESM should not affect that NCJS in any significant way. It is true that in some cases, you might not be able to use some NCJS from ESM, specially those modules adding dynamic exports, but that is not a BC problem IMO, because the importer has chosen to write their module in ESM.
Are you planning to unify the loader? or change the internals of the NCJS loader as it exists today? I might be missing some really important here. I can only say that we went over these issues many times, and we have found no evidence that those 3 rows could be problematic.
@caridy ... to be absolutely honest, that's what I'm trying to understand also. The more I look at this, the more I'm thinking that there's really less of a problem here than it would appear...
if we drop the ability for ESM to
import
NCJS, & instead provide ESM a means torequire(ncjs)
,
@chrisdickinson if we do this we end up in a strange situation where a dep tree may be ... ESM -> NCJS -> ESM
... that presents a large handful of effectively undefined behavior. What gets evaluated and when? :S
@caridy @jasnell Static Exports
( 1
) and Evaluation Order
(2
) are annoying, and are regressions of what the module system that we would be moving to is capable of. (And 2
could be considered bad trap for users... easier to get bit by timing order issues that are very hard to debug.)
If Async Loading
(4
) is not an issue, I have long asserted that Immutability/Idempotency of Modules
(3
) is the major problem.
This could "easily" (or... simple in concept, as it would function as much code currently expects) be solved if we can "link" non-default exports to a default Object export under the hood rather than using a Module Record.
This probably wouldn't be possible in browsers but I think that would be OK. If our loading differs anyways on the async part, I don't think parse-time linking in the same way is a hard requirement.
The only thing I can see that you loose at this is live bindings that act quite the same, but I think that is an acceptable limitation in the name of much better backwards compatibility and feature parity that doesn't seem to be reclaimable any other way...
Speaking of which, I think the "Object linking" thing could also solve some problem relating to import
-ing JSON that I / we forgot about? @bmeck does this sound familiar?
@Fishrock123 very familiar, that was the original plan with the Dynamic (not reflective) ModuleRecord bit in the EP. All non-ESM should use the same wrapper, so C++/JSON can use the same wrapper as NCJS (noted in https://github.com/nodejs/node-eps/pull/39).
@caridy @jasnell I would avoid this list unless we have clear specified goals. Most of the topics at hand here are subjective problems depending on what level of support you want for existing code (either transpiled ESM or NCJS) to consume/produce values from/to ESM.
Async loading breaks existing expectations of how ESM can be consumed by NCJS. Sync loading causes problems with spec (the bullet points listed above show odd or invalid behavior). This becomes increasingly important as an async dependency deep within a dep graph means that any consumer of the async dependency must use some form of async control flow. This effectively means that to have an ESM dependency you need to make your module wrapped in some async logic while waiting for the dependency.
Static import/exports breaks getting your export list after evaluation from NCJS. This breaks existing expectations of how ESM can consume NCJS. This relates to consuming Node's core modules.
Immutability/Idempotency of Modules relates to provided named imports to ESM from non-ESM sources.
If all of these breaks are considered fine, then there are no problems. https://github.com/nodejs/node-eps/pull/39 is a solution that assumes these breaks are not problems.
If all of these breaks are considered fine, then there are no problems. nodejs/node-eps#39 is a solution that assumes these breaks are not problems.
That is effectively why this thread exists. I am not sure those breaks are ok. The @nodejs/ctc needs to help provide opinion.
@Fishrock123:
if we do this we end up in a strange situation where a dep tree may be ... ESM -> NCJS -> ESM ... that presents a large handful of effectively undefined behavior. What gets evaluated and when? :S
I'm not sure I follow how that represents undefined behavior? Dynamic import()
within a Module or Script context is defined behavior (edit: or at least, it would be specified behavior), and the NCJS behavior on require
is already defined as well. Indeed, as you say, it would ping-pong between ESM evaluation and NCJS evaluation, but in a defined fashion. An entire ESM tree would evaluate, NCJS evaluates on top of the ESM tree during the evaluation phase at require-time. Subsequent ESM trees would load entirely after the current ESM lifecycle, if any.
@ljharb, @Fishrock123:
To what end must we support require('esm')
and import foo from 'cjs'
? What does not knowing authoring format get our users, concretely, and is it worth potentially diverging from browser ESM evaluation behavior in order to get that? Edit: Implicit in this statement is the following assumption: even if we can evaluate ESM synchronously, wouldn't this cause the same ESM tree to evaluate in a different order between Node and the browser?
This was largely in preparation for the TC-39 meeting that already happened, right? I'm going to close this but feel free to re-open or comment if the conversation should continue.
@chrisdickinson sorry for the delay - to what extent? I think 100%. Without that critical and transparent interop, users won't be able to migrate their modules in a backwards-compatible way, which will literally decimate the ecosystem by forking packages into "cjs" and "esm" - which would be disastrous while people still use many node and browser versions that don't support esm.
Some preliminary work to try to address the evaluation order to support interoperation: https://github.com/caridy/proposal-dynamic-modules
Note: This is for collaborators and especially the CTC to figure out. Comments that are deemed not productive will be removed. Do not tell me why "Node.js needs ESM". I am well aware of our operating constraints in the ecosystem.
At some point we'll need to decide something about ES Modules, at the current time it would be most productive if we could at least decide about what we do care about, is some sort of order or rankings.
That is: There are problems, some may not be resolvable, but in order to make progress we need to decide on what to try to resolve at the spec level.
See https://github.com/Fishrock123/node-eps/blob/b56ee228fc67b972daeba3df094e651bf4ede870/002-es-modules.md for a detailed explanation of the major issues.
import
-ing NCJS will always look different thanimport
-ing ESM.import { readfile } from 'fs';
require()
of ESM not possible.graceful-fs
v4 won't work if we ever move core modules to ESM.fs
to be quite honest.What sort of these things to we actually care about?
So far I've been collecting problem cases and asserting that we have no small amount of problems with ESM, but to make progress with it... it would be nice to know what points we care about most and focus on those.
Of course, new ones may come up.
/ cc @nodejs/ctc
Note this is somewhat in prep for next week's TC39 meeting, sorry I didn't post this sooner but there is time crunches all around and we don't really have the liberty of waiting until a "next week".
So far I know that the following Node.js people will be attending next week's TC39: @mikeal, @jasnell, @bmeck
(In addition I may be, but that isn't determined right now.)