-
Notifications
You must be signed in to change notification settings - Fork 27
Decide on what problem points for ES Modules we care about the most. #15
Comments
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.
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.
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
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.)
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
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.
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 I understand this area worse than the others so apologies if I completely missed the requirements.
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:
replies to @domenic:
The edge cases of this have been gone over several times and:
Heavily on the side that doing suck plucking is not a good path.
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).
Correct.
The problem is with order of evaluation and changing behavior. NCJS loads and evaluates top down, but placing |
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 |
Nothing explicitly forbids it, but parts of spec / semantics require async behavior in order to function as expected:
|
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 @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 |
@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 |
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 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 |
Maybe I should provide an analogy here to make sure that we are all on the same page. In node terms, Whether the evaluation on each of those dependencies 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 |
I note that:
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 |
@jasnell the only two invariants here for module A are:
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:
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:
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:
This does provide a possible path forward for at least part of these issues. One could easily imagine a case such as 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 |
@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... |
@chrisdickinson if we do this we end up in a strange situation where a dep tree may be |
@caridy @jasnell If 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 |
@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 nodejs/node-eps#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. 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. |
I'm not sure I follow how that represents undefined behavior? Dynamic To what end must we support |
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.)
The text was updated successfully, but these errors were encountered: