Closed jdalton closed 8 years ago
Let me know if you'd like more of the UJSG proposal brought over, things regrouped, or split out.
So I guess there are a couple of ways forward here:
The only problem with the second option is that we'd have to build the detection code ourselves without being able to rely on V8, I assume, perhaps that would be as simple (?) as doing a Module parse, which, if successful, could be inspected for import
and export
and then fail if those don't exist.
Also, this doesn't include the "modules.root"
property for fat packages, I would have thought that's a necessary part for a migration period (which could be quite long).
/cc @nodejs/collaborators - please chime in on this, it'd be great to have more than just the CTC engaged on the modules questions, I think we'd all appreciate more core team minds being applied to this than we currently have.
Since you're asking (and I'll keep it short), I would definitely wait for TC39's position on this. If an alternative solution comes out of this (again), we should probably re-evaluate. I see absolutely no reason to rush here. It's not like CommonJS is broken.
I don't think that parsing ourselves is a great idea, so detection part needs to be implemented in v8 first
@ronkorving even if we opt for waiting for TC39 to pass judgement, they may be looking to us to provide a signal wrt whether this is going to be something we'd even end up using. While the unambiguous grammar proposal serves more than just Node, we are the primary beneficiary and target of it. TC39 are obviously not so comfortable with the idea of changing a spec that they've already signed off on, but given the consternation around Node's preference for .mjs
, if TC39 hears from us that we may be willing to give up on .mjs
in favour of this if it gets through their hoops then they may be more likely to consider it. So it's a bit of a multi-party dance and the priority now is to get maximal input from Node Collaborators so we can figure out whether we are even willing to consider this path.
I wasn't gonna touch the topic of preference of the actual solution, but I for one much prefer this over a new file extension.
I'm -1. This is solution is not significantly better (or not better at all), but is much harder to implement and affects performance
@vkurchatkin How does it effect performance? It doesn't seem like this would actually significantly impact startup time, but we'd have to benchmark to say that it would, which is pretty much impossible without V8 implementing it.
@Fishrock123 I wonder then how
Performance is generally on par with existing CJS module loading
is achieved
@Fishrock123 it'll require a full double-parse unless the module type being loaded is the same as the default goal being parsed by the version of Node.js being used, and we can't really cache that between processes, only within a process. The .mjs
proposal only costs a few extra stat
calls.
it'll require a full double-parse
This really depends on the implementation. It's totally possible not to drop away all the parsing results, but only to delay strict early errors + things like octal literals until the end of the parsing (and, because V8 already uses lazy parsing, it should result in pretty small if no overhead).
I guess that's a call for V8 to make re optimisation, my understanding was that the two goals are handled completely separately in the current incarnation of Modules that V8 has implemented (but not shipped). It'd be nice if this was all handled upstream from Node but I doubt we'll get such a free ride.
@jdalton @joshgav can we pull in some ChakraCore folks in here as well to have them comment on where this might be implemented in a ChakraCore-backed Node and how much Node could get for free if this became part of the spec?
@RReverser
It's totally possible not to drop away all the parsing results, but only to delay strict early errors + things like octal literals until the end of the parsing (and, because V8 already uses lazy parsing, it should result in pretty small if no overhead).
Note that e.g. a top-level await(fo);
would have a different meaning in scripts and modules, and it might happen that this would be correct in both of those, but have different results.
would have different meaning in scripts and modules, and it might happen that this would be correct in both of those, but have different results
Sure, but I guess you're talking about runtime semantics here, not parsing? Note that for distinguishing modules only parsing stage is important.
@RReverser In that example, await
will be a function name for scripts, and it would likely be a keyword in modules case.
and it would likely be a keyword in modules case
Ah right. Another one to the list of delayed elements (like octal literals), although I agree it's somewhat trickier. (Btw, AFAIR it's not even part of the current proposal, only a future consideration?)
This really depends on the implementation. It's totally possible not to drop away all the parsing results, but only to delay strict early error
It can't be done outside of engine, and engines will probably do it only if it's a part of Ecma-262
It can't be done outside of engine, and engines will probably do it only if it's a part of Ecma-262
Just saying that it can be done efficiently on the level of engine, but if not, we can just rely on own parsing pass, which will be less efficient, but still work. Personally, I really like this idea independently of specific implementation details.
@rvagg modules.root
is removed in prep for talking with TC39 and to simplify spec. We know a solution and I can make a separate EP for that which won't cause extraneous discussion with TC39.
We do need v8 buy in for the case of export {}
since it does not export anything we cannot detect that it exists in the code. import ...
always produces a request to load some code, and export {name}
/export default
always add a value to the list of exports.
We have a few contacts at ChakraCode we have talked to in private about the steps necessary for this, I had a very specific call with @bterlson about parsing and bytecode caching. It was held under the idea of a grammar change, but they seemed pretty unfazed by things.
@RReverser v8 has said it would be non-trivial for us to get a parser that does both modes. It may be possible to get them to provide it, but not in the near future.
A key part to understanding the way to get max perf is:
package.json
and you only parse once.All this said, I am heavily on the side of a file extensions unless this removal of ambiguity lands in spec. File extensions do not scale like parsing does; but, extending the spec has gotten us arguments about doing things not to spec in the past. I fear we will get bitten in some way where the spec does not consider the implications outside of browsers like "uncaughtRejection" is currently dealing with.
To echo a bit of @bmeck, after meeting with the CTC it became clear that modules.root
is really its own thing, separate from the UnambiguousJavaScriptGrammar proposal. It has applications for both module formats and has plenty of other things to debate and iron out on its side.
Performance is not a big concern. Investigations were had, benchmarks run (@trevnorris), and the numbers showed that even without caching, and worse case double parsing many many files, that the performance impact was not a blocker.
I can't see a future in which engines don't help Node achieve its goals. V8 and Chakra are already optimizing specifically for Node and are invested in Node on several fronts. SpiderMonkey has experiments in the Node space as well.
As for the TC39, I'm optimistic. This July will be @bmeck's first meeting with them as a TC39 member. It's going to be great to have Node represented there. I'm sure something can come out of the meeting to avoid the "doing things not to spec" arguments.
While iterating on this proposal @bmeck and I kept coming back to keeping it simple. What came out was something that can scale as more parse goals are added, something that supports stdin, something that doesn't require lots of metadata, and something that "just works" for developers without the ecosystem concerns of the current proposal.
Note that e.g. a top-level await(fo); would have a different meaning in scripts and modules
Non-module code is allowed top-level await?
@trevnorris await
is not a reserved word in Script, so it could be anything a normal identifier could refer to, function, variable, etc.
@rvagg
@jdalton @joshgav can we pull in some ChakraCore folks in here as well
@jdalton is working on it, in the meantime /cc @aruneshchandra @nodejs/chakracore
@rvagg After a brief discussion with @jdalton on this proposal I believe that, for supporting Node, exposing a new parse API in ChakraCore that takes some source text and returns whether it has an Import or Export declaration, is doable and not likely to be expensive in terms of implementation and performance.
:metal:
I pinged @ajklein (V8) who echoed that providing an API for Node would be doable. From chatting with both Chakra/V8 folks it looks like the new API would not have the ES spec change as a hard requirement either. An engine API like this could be useful beyond Module detection too, e.g. detecting something like asm.js as a goal.
@jdalton So wait, do we not need to change the spec then, or does changing the spec just make the parseability faster?
@Fishrock123 The spec change isn't really about performance or helper API implementation. See the ecma262-solution section.
@aruneshchandra
takes some source text and returns whether it has an Import or Export declaration
@rvagg @Fishrock123: Is this all Node.js needs? Or do we want to know that Chakra and V8 will parse a module without import/export keywords as a script? I think that would be the spec change.
@joshgav I am heavily, and I do stress heavily, against doing parsing while ambiguity remains in the spec. It leads to surprises for developers in non-Node environments, and lovely comments about how Node is non-standard by spec developers. It also has led to situations where spec has not considered implication on Node since behavior is seen as non-standard.
We can take the parsing approach if given an API, but I will fight it will all my might if it is not endorsed by TC39 / spec.
@bmeck
It leads to surprises for developers in non-Node environments
Code moving from node to the browser (current npm workflow) will continue to work regardless of spec change, so that's nice.
It also has led to situations where spec has not considered implication on Node since behavior is seen as non-standard.
I think having you on on the TC39, representing Node, will help avoid issues like that going forward :)
We can take the parsing approach if given an API,
🙌
Reading through this, some reactions:
.mjs
.modules.root
: This is strictly OT for this PR, but it would be valuable to talk about this topic with @isaacs. I believe he implemented something like this behavior in an early version of Node, and the resulting confusion led him to remove the behavior.@chrisdickinson
I agree wholeheartedly with @bmeck: we should not implement this approach (or merge this PR)
I don't believe @bmeck is suggesting the PR should not be merged.
Correct, I think it should be merged in order to show commitment (just like the file extension was). I doubt we can make easy progress without a vote of confidence prior to getting support. We are not moving to ACCEPTED
and as such the proposal will continue to be open to change / reverting.
To rephrase: I agree with @bmeck that committing to this approach should be contingent on TC39 blessing the approach (by recommendation or spec change) and V8 giving us a CompileModule
API corresponding to that recommendation or change.
That said, assuming that this EP represents the CTC's intent with regards to modules, we should either re-include the .mjs
bits (noting that we prefer unambiguous parsing if it becomes available to us) or wait to merge this PR until we know whether or not unambiguous grammar will become available to us.
As requested from today's CTC meeting, I moved a bit of the ecma262-solution section over to this PR.
Thanks to @jdalton for his comments today at the CTC meeting about "modules.root"
being removed and the "fat packages" concern maybe being misplaced—I hadn't thought about it this way but I can see that accommodating fat packages like this might lead to sub-optimal outcomes, with the runtime taking different paths depending on which version it loads and that it might be best to leave it up to users to figure out their own migration timing for a switch from transpiling to native.
@nodejs/collaborators if possible, we'd like to have a vote at the next @nodejs/ctc meeting and we'd like input from the broader group before that can happen. Please review the change this PR makes, and comment here if you have any questions or have any concerns that we should hold up the process to resolve. A -1
is an acceptable comment but please give some justification for that in case there are ways your concerns can be resolved and to help guide the CTC to a final decision. Collaborators are still the first gate in the decision-making process so please engage!
The basic question here goes something like this:
If TC39 agrees to revisiting the Modules spec to accommodate the needs spelled out in https://github.com/bmeck/UnambiguousJavaScriptGrammar and the ability to detect filetype is made possible by our JS engine(s) then we would use such a mechanism to load, detect and run .js
files whether they are of Script or Module type; i.e. discarding the .mjs
detection option.
As for how to reflect this conditionality in the doc, I like @chrisdickinson's suggestion of including the .mjs
option in the doc still as a fallback case if we don't get what we need from TC39 and making it clear that the decision to go either way is contingent on having a spec change. Let's be absolutely clear about the path forward, which does have a fork in it.
I can't say I understand at the current time what "modules.root"
was removed, but that definitely makes me a bit happier...
@rvagg
As for how to reflect this conditionality in the doc, I like @chrisdickinson's suggestion of including the .mjs option in the doc still as a fallback case if we don't get what we need from TC39 and making it clear that the decision to go either way is contingent on having a spec change.
Cool. I can add a link to the current .mjs
revision. (updated ✔)
Let's be absolutely clear about the path forward, which does have a fork in it.
We should clarify why the fork, Node restricting ES modules to having at least one import/export and browsers not, is an issue. Especially because:
I've been a bit wiggly with the proposal text:
A specification change or at least an official endorsement of this Node proposal would be welcomed.
It's wiggly because at the time the root concern seemed to be upset emails/tweets about not following the spec, so a :+1: of some kind from TC39 would provide a way to avoid that. I think clarifying the issue would help the wiggles :)
I am more concerned with things progressing in spec without consideration to the approach taken by Node to things, unhandledRejection is something that falls in that category of having to deal with things because it was not well understood that Node had a different set of expectations. The emails and discussions are also a concern, but show deeper problem of "non-standard" behavior not being taken into as much consideration as spec progresses.
@bmeck Would both of your concerns be tackled by your joining of the TC39 to champion for and highlight issues that affect Node?
TC39 is consensus based, I could influence things but not guarantee that any non-standard behavior would be respected in the future.
I think Node representation at the TC39 goes a long way. I'm not sure guarantees are guaranteed. For example, I relied on a the spec'ed Reflect.enumerate
path but it was pulled after being added and rubber stamped.
If design choices that effect Node are a concern, having friends like Chakra/V8 can help. Node/Microsoft/Google/jQuery-Foundation should be able to elevate concerns. I see Node being a part of the TC39 as a way to get things like the Node package ecosystem taken into account when new built-in APIs are proposed, in much the same way folks look to browsers for API conflict investigations.
Noting here for clarity that this was added in response to the request above:
Note: While the ES2015 specification does not forbid this extension, Node wants to avoid acting as a rogue agent. Node has a TC39 representative, @bmeck, to champion this proposal. A specification change or at least an official endorsement of this Node proposal would be welcomed. If a resolution is not possible, this proposal will fallback to the previous
.mjs
file extension proposal.
We'll try and move for a vote at the CTC meeting today unless anyone has reason to hold it up further.
:tada:
Thanks much! Landed in 86e0241ddd896fd04f718bb4460a11b9e47dfc74.
This PR replaces section 5.1 of the ES6 Module Interoperability proposal with the UnambiguousJavaScriptGrammar proposal.
TL;DR
.js
files by parsing as one grammar and falling back to others