Open nicolo-ribaudo opened 11 months ago
I was planning on proposing this for Stage 2.7 at the April meeting, and I remembered just a few days ago (because I was pinged for a review) that editor reviews are needed for Stage 2.7 and not for Stage 3.
The spec text at https://tc39.es/proposal-defer-import-eval/ is complete and is what I plan to propose. Regarding the open PRs in this repo:
import defer
. They can be thus ignored.I know that this ping is coming way too late, so I understand if it's impossible for you to take a look before plenary :)
My review can be considered complete after https://github.com/tc39/proposal-defer-import-eval/issues/38 is resolved, with the PR in https://github.com/tc39/proposal-defer-import-eval/pull/39 and https://github.com/tc39/proposal-defer-import-eval/pull/35 already landed. The implementation approach is really neat, nice work.
This looks good to me, thank you for the good work here.
@bakkot @michaelficarra @syg I'm planning to re-propose this for 2.7 at the next meeting, I would appreciate if you could find some time to review the spec text.
If you already took a look at this last time, there have been two PRs to handle a bug that @guybedford found with re-entrancy and top-level errors: https://github.com/tc39/proposal-defer-import-eval/pull/39 and https://github.com/tc39/proposal-defer-import-eval/pull/43
I've had another look, I think this is good for 2.7.
@nicolo-ribaudo Why do you use an early error to restrict ImportClause in ImportDeclaration to NameSpaceImport when defer
is present? Why not just add an alternative to ImportDeclaration?
@nicolo-ribaudo There's also a misspelling of [[Specifer]]
that occurs multiple times.
edit: I'll just turn this into a review feedback comment.
evaluatePromise
doesn't need to be declared in step 8.c. You can have more than one declaration (at the points where you currently assign it) as long as all paths to the first use pass through exactly one declaration.GatherAsynchronousTransitiveDependencies
. It looks like we can easily avoid it with some list-concatenations, right?
Before Stage 3, we still need to investigate multiple aspects of the proposal. Each topic should be discussed in its own issue, but I'm writing all of them here to keep track of the progress.
TODOs
Early errors (related to https://github.com/tc39/proposal-defer-import-eval/issues/9)How critical are early errors? Is it ok to defer them until execution, similarly to what happens with dynamic import? This would allow some systems to entirely defer loading modules. EDIT: After talking with people working on various runtime implementations, it's clear that those implementations would still be able to avoid loading by pre-computing metadata about files.Deferred re-exports (related to https://github.com/tc39/proposal-defer-import-eval/issues/18)(#30, #31) It would be useful to support something likeexport defer { f } from "./foo"
, that causes the execution of./foo
only iff
is actually imported form this module. EDIT: The surface area for re-exports is much larger due to the tree-shaking semantics, I plan to ask the committee to leave it at stage 2 for longer.import source
is now stage 3, we should probably add dynamicimport.defer()
to this proposal similar toimport.source()
.Reviews