-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normative: Add JSON modules #3391
base: import-attributes
Are you sure you want to change the base?
Conversation
|
||
<emu-clause id="sec-createsyntheticmodule" type="abstract operation"> | ||
<h1> | ||
CreateSyntheticModule ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This AO is only used once, it's not used in any host spec, and its implementation is a single line. Can we inline it?
It seems like V8 has an API based on this AO (https://v8docs.nodesource.com/node-13.2/df/d74/classv8_1_1_module.html#aeaa8c3cfb75478b9403be3d3955bb338), but nothing prevents implementations to keep providing this sort of constructor for Synthetic Module Records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I would inline this and remove the references to it.
</emu-clause> | ||
|
||
<emu-clause id="sec-smr-module-record-methods"> | ||
<h1>Implementation of Module Record abstract methods</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting a note from the original authors of the proposal:
I find having this wrapping sub-clause cleaner and suggest we do the same for Source Text Module Records in the main spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, they should match.
|
||
<p>The following are the concrete methods for Synthetic Module Record that implement the corresponding Module Record abstract methods defined in <emu-xref href="#table-abstract-methods-of-module-records"></emu-xref>.</p> | ||
|
||
<emu-clause id="sec-smr-LoadRequestedModules" type="concrete method"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an open PR in the proposal changing these smr
shorthands to the longer sec-synthetic-module-record-LoadRequestedModules
. What is the editorial preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am completely unopinionated on IDs. This ID is fine.
</dl> | ||
|
||
<emu-alg> | ||
1. Let _json_ be ? Call(%JSON.parse%, *undefined*, « _source_ »). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should steps 2-9 of JSON.parse be extracted to a new AO that we can call here, or is this ok?
Related: #1012 and https://infra.spec.whatwg.org/#parse-a-json-string-to-a-javascript-value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mildly prefer the AO (after #3394 lands), but that refactoring can happen after this PR.
9e016c1
to
749ca87
Compare
@tc39/ecma262-editors Remember that this is going for stage 4 at the next meeting :) |
@@ -28495,12 +28698,19 @@ <h1> | |||
</ul> | |||
<p>and it performs FinishLoadingImportedModule(_referrer_, _moduleRequest_, _payload_, _result_) where _result_ is a normal completion, then it must perform FinishLoadingImportedModule(_referrer_, _moduleRequest_, _payload_, _result_) with the same _result_ each time.</p> | |||
</li> | |||
<li> | |||
<p>If _moduleRequest_.[[Attributes]] has an entry _entry_ such that _entry_.[[Key]] is *"type"* and _entry_.[[Value]] is *"json"*, the host environment must perform FinishLoadingImportedModule(_referrer_, _moduleRequest_, _payload_, _result_), where _result_ is either the Completion Record returned by an invokation of ParseJSONModule or a throw completion.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<p>If _moduleRequest_.[[Attributes]] has an entry _entry_ such that _entry_.[[Key]] is *"type"* and _entry_.[[Value]] is *"json"*, the host environment must perform FinishLoadingImportedModule(_referrer_, _moduleRequest_, _payload_, _result_), where _result_ is either the Completion Record returned by an invokation of ParseJSONModule or a throw completion.</p> | |
<p>If _moduleRequest_.[[Attributes]] has an entry _entry_ such that _entry_.[[Key]] is *"type"* and _entry_.[[Value]] is *"json"*, the host environment must perform FinishLoadingImportedModule(_referrer_, _moduleRequest_, _payload_, _result_), where _result_ is either the Completion Record returned by an invocation of ParseJSONModule or a throw completion.</p> |
<li> | ||
The operation must treat _payload_ as an opaque value to be passed through to FinishLoadingImportedModule. | ||
</li> | ||
</ul> | ||
|
||
<p>The actual process performed is host-defined, but typically consists of performing whatever I/O operations are necessary to load the appropriate Module Record. Multiple different (_referrer_, _moduleRequest_.[[Specifier]], _moduleRequest_.[[Attributes]]) triples may map to the same Module Record instance. The actual mapping semantics is host-defined but typically a normalization process is applied to _specifier_ as part of the mapping process. A typical normalization process would include actions such as expansion of relative and abbreviated path specifiers.</p> | ||
|
||
<emu-note> | ||
<p>The above text implies that hosts *must* support JSON modules imported with `type: "json"` (if it completes normally), but it doesn't prohibit hosts from supporting JSON modules imported with no type specified.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<p>The above text implies that hosts *must* support JSON modules imported with `type: "json"` (if it completes normally), but it doesn't prohibit hosts from supporting JSON modules imported with no type specified.</p> | |
<p>The above text requires that hosts support JSON modules when imported with `type: "json"` (and HostLoadImportedModule completes normally), but it does not prohibit hosts from supporting JSON modules when imported without `type: "json"`.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise.
This pull request builds on top of #3057.
I mostly ported the spec text as-is, taking care of integrating editorial changes from the PRs open in the repo, but I have a few comments / questions for the editors.