Editorial: refactor classic and module scripts to be more alike #2972
Conversation
domenic
added
clarification
|
Preview of the output with these changes: https://module-fixes-creation-vaieylcnns.now.sh Places to review are "create a module script", "create a classic script", the definition of the "script" struct, "run a classic script", and "run a module script". |
source
Outdated
|
|
||
| </dd> | ||
| <dd><p>Either a <span>Script Record</span>, for <span data-x="classic script">classic | ||
| scripts</span>, or a <span>Source Text Module Record</span>, for <span data-x="module |
There was a problem hiding this comment.
This or seems redundant with the final or.
domenic
removed
the
do not merge yet
|
I have confirmed this provides a stable foundation going forward, so removing "do not merge yet"; review appreciated. |
|
I guess it should still be "do not merge yet" since it needs to be on top of #2971. |
domenic
added
the
do not merge yet
domenic
force-pushed
the
module-fixes-3
branch
from
cf76523 to
5a4edab
Compare
| data-x="js-ScriptEvaluation">ScriptEvaluation</span>(<var>result</var>).</p></li> | ||
| <ol> | ||
| <li> | ||
| <p>If <var>rethrow errors</var> is true and <var>script</var>'s <span>muted |
There was a problem hiding this comment.
Shouldn't this be "muted errors is false"?
(The condition here and the condition at Lines 87206--87208 are the same and at least either of them should be fixed.)
There was a problem hiding this comment.
Good catch, thank you!! Pushing fix now.
source
Outdated
| <li> | ||
| <p><i>Error</i>: At this point <var>result</var> must be an exception. Perform the following | ||
| steps:</p> | ||
| <li><p>Rethrow <var>evaluationStatus</var>.</p></li> |
There was a problem hiding this comment.
Shouldn't we rethrow evaluationStatus.[[Value]] instead of evaluationStatus?
There was a problem hiding this comment.
I guess that's probably more understandable. This interface between the ES spec and HTML is a bit confusing as to what terminology to use; I meant "rethrow evaluationStatus" as "return evaluationStatus, which is equivalent to rethrowing its [[Value]]". But your phrasing is better. I will change.
|
@hiroshige-g does this LGTY? (When rebased on #2971, if #2971 LGTY.) |
|
lgtm, once rebased. |
This makes "create a classic script" work the same as "create a module script". Both now parse source text into the appropriate record (viz. Script Record or Source Text Module Record), storing the result, or any parse error, for later use by "run a X script". As a consequence, this expands the base "script" struct to include "record" and "parse error" fields, which were previously module-specific. In general, this alignment will help greatly when working on dynamic import(), which per https://github.com/tc39/proposal-dynamic-import/blob/23cf15a8ccef0c7174b504b33fb6b2b9be1ebecd/HTML%20Integration.md will move more things into the shared base struct. Note that for now, "parse error" is somewhat of a misnomer for modules, as we propagate dependency errors into this field. This is hinted at by leaving the name for that algorithm, "set a pre-instantiation error", intact. We anticipate clearing up this confusion in subsequent commits, by changing error propagation, but this was left as such in order to keep this commit purely editorial. Along the way, this: * Fixes a few remaining references to module trees, as opposed to module graphs. * Changes "muted errors" from a flag into a boolean. * Modernizes the algorithm style for both create algorithms, making them take named parameters, stop using goto-style jumps, and use booleans instead of flags. * Fixes the spec to report errors while inside the "prepare to run a script" / "clean up after running script" block. * Fixes several wrong variable references within these algorithms.
domenic
force-pushed
the
module-fixes-3
branch
from
03ee730 to
3d9a230
Compare
domenic
removed
the
do not merge yet
This makes "create a classic script" work the same as "create a module
script". Both now parse source text into the appropriate record (viz.
Script Record or Source Text Module Record), storing the result, or any
parse error, for later use by "run a X script".
As a consequence, this expands the base "script" struct to include
"record" and "parse error" fields, which were previously
module-specific.
In general, this alignment will help greatly when working on dynamic
import(), which per
https://github.com/tc39/proposal-dynamic-import/blob/23cf15a8ccef0c7174b504b33fb6b2b9be1ebecd/HTML%20Integration.md
will move more things into the shared base struct.
Note that for now, "parse error" is somewhat of a misnomer for modules,
as we propagate dependency errors into this field. This is hinted at by
leaving the name for that algorithm, "set a pre-instantiation error",
intact. We anticipate clearing up this confusion in subsequent commits,
by changing error propagation, but this was left as such in order to
keep this commit purely editorial.
Along the way, this:
graphs.
take named parameters, stop using goto-style jumps, and use booleans
instead of flags.
script" / "clean up after running script" block.
@nyaxt or @hiroshige-g to review.
On top of #2971.
Marking "do not merge yet" as this should probably only be merged as part of the final module revisions package. That is, more PRs are coming on top of this one. But this seems like a nice separate chunk of work, editorial only, which can be reviewed independently.