Stage 3 reviews!

[domenic]

Here are the people who have signed up as reviewers for this spec that need to sign off before it can get to stage 3, per the notes:

• [x] @jasnell
• [ ] @allenwb (reviewed but not approved)
• [x] @caridy

And of course the editor:

• [x] @bterlson

Also, we have one "unofficial reviewer" whose feedback would be welcome:

• [x] @ajklein

Useful resources, perhaps:

• A convenient spec text link
• An example of HostPrepareImportedModule, for HTML
• https://github.com/domenic/proposal-dynamic-import/issues/4 is the only currently-outstanding issue, and it's about the spec mechanics of having "spec asynchronous" algorithms; thoughts welcome.

[caridy]

@domenic thanks a lot for getting this ready for stage 3, it is simple enough and small enough IMO. My only comment is about HostPrepareImportedModule, I think the name is misleading, I got confused at first. I guess I was associating it with HostResolveImportedModule but they are very different. I don't have an actual recommendation to name it, but it is something that we should probably look at.

👍

Btw: it is also the first time we have an abstract operation in 262 using NewPromiseCapability aside from the promise API definition itself :)

[domenic]

Awesome! Filed #18 to discuss the name. Much appreciated!

[ajklein]

Overall looks good. A few comments (and apologies if there are dumb questions in here, it's my first time reviewing one of these specs to the extent required by stage 3):

1. The grammar is not complete. Instead, there's a parenthetical: "(Also, many other instances of CallExpression need to get ImportCall added to them.)". How feasible would it be to include all such modifications before the meeting? Presumably that work will have to be done at some point.
2. It seems strange that an exception thrown during evaluation of the AssignmentExpression results in a thrown exception at the callsite, while a failure in the ToString call results in a rejected promise. I would have expected a failed ToString to throw immediately just the same. Is your intuition that the ToString operation is happening "inside" the function body of import? This distinction seems unimportant to me, given that import() isn't really a function call anyway, but I could be convinced.

[domenic]

How feasible would it be to include all such modifications before the meeting? Presumably that work will have to be done at some point.

Happy to do that. It's pretty mechanical Ctrl+Fing, but I can indeed list them.

[domenic]

(oops, commented too soon)

Is your intuition that the ToString operation is happening "inside" the function body of import? This distinction seems unimportant to me, given that import() isn't really a function call anyway, but I could be convinced.

This is exactly the intuition. In particular,

import((() => { throw new Error(); })()).then(() => { });

should behave the same as

const arg = (() => { throw new Error(); })();
import(arg).then(() => { });

i.e. it should throw immediately at the call site. But the argument processing, just like any other invalid argument, should cause a rejected promise. I can see how it's a bit different for a syntactic form, but I think we should treat it more like a function than not. The same argument about not wanting to duplicate error-handling logic between catch (e) { ... } and .catch(e => ...) applies.

[jasnell]

Overall this looks solid. Other than the fact that Node.js would currently have difficulty performing the module loading completely asynchronous, it should be possible to abstract away that limitation behind the Promise interface so that there is little real impact on the actual code beyond performance. The fact that this would be limited to Script and Module Records is ++.

Pinging @bmeck for his input.

[domenic]

Added the missing static semantics. It turns out there were only three locations where it needed to be added, so I am glad @ajklein prompted me to do so.

[domenic]

Reviewers should be made aware that there have been some structural changes as a result of discussions in #24, which make the previously-imprecise "asynchronous completion value" mechanisms much more precise. This has changed a decent amount of the text, but hasn't really made any normative changes, so hopefully your reviews are still valid :). See https://github.com/tc39/proposal-dynamic-import/commit/b8de8717b67e0c9b46fe29b4ae324cb9bbc00084 for the changeset.

[bterlson]

Feedback:

• "The abstract operation must always complete with undefined": I'd say, "must always complete normally with undefined".
• Third bullet of 1.1.2 should say "fails" instead of "succeeds".
• Fifth bullet of same, given same arguments is incorrect since the signatures don't match. Say given the same referencingScriptOrModule specifier pair?
• Sixth bullet of same, I don't know what "manipulate" means. It seems like the only important aspect here is that the promise is not resolve or rejected this turn. Therefore, maybe just say that rather than trying to overspecify?
• Last paragraph of 1.1.2, don't know what "during an initial step" means.
• DynamicImportPreparationFinishedJob feels like a troll ;) is "Preparation" necessary here?
• 2.2.1 step 2, "i.e. is not null" seems superfluous to me.

Signing off, though curious to see where Allen is going with the operator formulation.

[domenic]

Thanks @bterlson! I took care of all of those, except I think "i.e. is not null" is still valuable since it makes it clear why we're doing the assert, IMO.

[bterlson]

Would you argue that all asserts should say "(i.e. )"? Eg. "Assert: Type(V) is Reference (i.e. not any other value)" or "Assert: Desc is a Property Descriptor (i.e. not an object or any other value)"? Personally I wouldn't want such a change.

[domenic]

No, just in the cases where you look at it and go "that's weird; what else could it possibly be?" To me it's a little surprising that GetActiveScriptOrModule() could return anything but a Script or Module so I found the i.e. helpful.

[allenwb]

I think the functionality described by this proposal is a good idea but there are still issues that I think need to be resolved before it is ready for Stage 3.

1. 23 concerns the syntax used for this operation and suggests use of an prefix import operator instead of a pseudo call-like syntax. There are reasonable pro/con arguments for each approach. A choice needs to be made by TC39 consensus prior to advancing the proposal.

2. 26 raise a concern that the import operator may provide a covert communications channel that can penetrate membranes. (The lack of a way to eval modules may be preventing this from already being a problem for static imports). Whether this is an actual problem and if so, whether a solution exists, needs to be resolved before advancing.

3. 24 (and #4) raises concerns about the specification mechanism used to describe the (potentially asynchronous semantics) of dynamic import. I think we have a shared conceptual understanding of ab appropriate semantics but I don't think the proposed spec. text fully or correctly captures that semantics yet. Perhaps this doesn't have to be a stage 3 blocker but stage basically means we want implementation work to start and that seems hard to do if we can't yet describe the semantics that needs to be implemented.

[domenic]

Thanks @allenwb! I agree we need to resolve #23 and, if there is an actual attack, #26, before proceeding. I think those will be resolved at the meeting. For #24, I personally think the current spec text is good to go, but if you're concerned about implementations not being able to understand its semantics correctly, I'd love to hear from any implementers with concerns.

[allenwb]

Note that I removed the check next to my name. I have completed my review but I have not unconditionally said that it should advance to stage 3.

[domenic]

That's fine. The check is meant to signify the completion of review, but I'm happy to let you check or uncheck the boxes as desired.

[allenwb]

"sign off" is the questionable language.