Skip to content
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

JSON module support #4407

Merged
merged 11 commits into from May 17, 2019
Merged

JSON module support #4407

merged 11 commits into from May 17, 2019

Conversation

@littledan
Copy link
Contributor

@littledan littledan commented Mar 2, 2019

This patch provides JSON modules as a single default export, with
parse errors checked before instantiating the module graph.

Note, editorially, it's unclear whether JSON modules should be
considered a type of "module script", with a settings object, fetch
options, base URL, etc or not. This patch considers them "module
scripts", but leaves those record fields unset (as they are unused).

This patch is based on
tc39/proposal-built-in-modules#44
which hasn't landed yet, so the references are a bit awkward, and
this patch should not land until that one does.

Closes #4315
Closes WICG/webcomponents#770


/acknowledgements.html ( diff )
/infrastructure.html ( diff )
/scripting.html ( diff )
/timers-and-user-prompts.html ( diff )
/webappapis.html ( diff )

source Show resolved Hide resolved
Loading
@annevk
Copy link
Member

@annevk annevk commented Mar 3, 2019

This also resolves WICG/webcomponents#770.

Loading

@littledan
Copy link
Contributor Author

@littledan littledan commented Mar 3, 2019

Once we settle on a wording here, it would be very easy to specify CSS modules, if we're settled on @justinfagnani's proposed semantics to give it a single default export of the parsed stylesheet. I don't see any open questions on the HTML side.

Loading

source Show resolved Hide resolved
Loading
Copy link
Member

@domenic domenic left a comment

Very interesting. After seeing the results, I'm quite unsure about calling JSON modules "scripts", although I would have suggested that course of action beforehand.

Implicitly, this seems to add a bunch of "assert: X is not a JSON module script" at the beginning of tons of algorithms that take a script. And I'm not sure all those asserts are satisfied, e.g., it seems like by doing <script type="module" src="foo.json"></script> we could end up at https://html.spec.whatwg.org/#execute-the-script-block with the script's script being a JSON module script.

In general, I think there are multiple direct and indirect callers of "fetch a single module script" which assume they are getting back something with all the appropriate struct fields set.

Probably these callers at least need a settings object. I'm unsure about the other fields.

An alternate approach would be to not have a "script" for JSON modules, and only have something you can put in the module map. I'm not sure if that would be better or worse. Probably worse...

Loading

source Show resolved Hide resolved
Loading
script">module scripts</span>; or null. In the former two cases, it represents a parsed script;
script">module scripts</span>; a <span>Synthetic Module Record</span> for <span
data-x="JSON module script">JSON module scripts</span>; or null. In the
former two cases, it represents a parsed script; in the third case, a parsed JSON document;
null represents a failure parsing.</p></dd>
Copy link
Member

@domenic domenic Mar 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to leave "a parsed script" covering all three cases, since we still consider these scripts.

Loading

@littledan
Copy link
Contributor Author

@littledan littledan commented Mar 6, 2019

Thanks for the review. In ced9b08 , module scripts are renamed JavaScript module scripts, and I caught a number of errors going through and making this change; added a number of asserts, and re-added the settings object.

Loading

source Outdated Show resolved Hide resolved
Loading
source Outdated Show resolved Hide resolved
Loading
source Outdated Show resolved Hide resolved
Loading
source Outdated Show resolved Hide resolved
Loading
@domenic domenic mentioned this pull request May 4, 2019
source Outdated

<li><p>Upon rejection of <var>jsonPromise</var> with <var>reason</var>, set <var>script</var>'s
<span data-x="concept-script-parse-error">parse error</span> to <var>reason</var>, and return
<var>script</var>.</p></li>
Copy link
Member

@Ms2ger Ms2ger May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't return anything to the caller of this algorithm from within a rejection/fulfillment handler.

Loading

Copy link
Contributor Author

@littledan littledan May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, some other pieces of spec text use "asynchronously" to get through this. Do you think that would be possible here?

Loading

Copy link
Member

@domenic domenic left a comment

I saw the update today to do things arguably more correct, and thought I'd do a pass. My conclusion is that you should go back to the incorrect way of consuming the body :)

Loading

source Outdated Show resolved Hide resolved
Loading
source Outdated Show resolved Hide resolved
Loading
source Outdated Show resolved Hide resolved
Loading
source Outdated Show resolved Hide resolved
Loading
source Outdated Show resolved Hide resolved
Loading
Copy link
Contributor Author

@littledan littledan left a comment

Thanks for all the suggestions, @domenic. I've done my best to apply them; feel free to push more corrections if you have ideas.

Loading

source Outdated Show resolved Hide resolved
Loading
source Outdated Show resolved Hide resolved
Loading
@littledan littledan changed the title [WIP] JSON module support JSON module support May 6, 2019
littledan added 8 commits May 7, 2019
This patch provides JSON modules as a single default export, with
parse errors checked before instantiating the module graph.

Note, editorially, it's unclear whether JSON modules should be
considered a type of "module script", with a settings object, fetch
options, base URL, etc or not. This patch considers them "module
scripts", but leaves those record fields unset (as they are unused).

This patch is based on
tc39/proposal-built-in-modules#44
which hasn't landed yet, so the references are a bit awkward, and
this patch should not land until that one does.

Closes whatwg#4315
This patch creates a common concept of a "module script", unifying
JavaScript module scripts with JSON module scripts (and future concepts
like style sheet module scripts, WebAssembly module scripts, etc.)
The name change includes the following fixes:
- Clarify that <link rel=modulepreload> works for JSON module scripts.
- Check for Cyclic Module Records in fetching module descendants
  and finding the first parse error.
- Initialize the settings object and error to rethrow in JSON module
  scripts, both of which are used by algorithms in HTML.
- Add various asserts to make it clear why we can use things when we do.

Module scripts may be renamed to simply "modules" in a future patch.
- Tweaks to <script> element stuff
- Add example
- Add Myles to acks for raising whatwg#4315.
- Tighten definition of JS and JSON module script a bit
domenic
domenic approved these changes May 7, 2019
Copy link
Member

@domenic domenic left a comment

OK, this LGTM. Remaining things:

I'll tag "do not merge yet" for the last one.

Loading

source Show resolved Hide resolved
Loading
source Show resolved Hide resolved
Loading
source Show resolved Hide resolved
Loading
- Delete double space
- Change my name from "Dan" to "Daniel" in acknowledgements
domenic added a commit to whatwg/webidl that referenced this issue May 17, 2019
Based on text written by Domenic at tc39/proposal-built-in-modules#44. These will be used by HTML in whatwg/html#4407.
@domenic domenic merged commit db03474 into whatwg:master May 17, 2019
2 checks passed
Loading
@littledan littledan mentioned this pull request May 19, 2019
2 tasks
Ms2ger pushed a commit to web-platform-tests/wpt that referenced this issue May 20, 2019
Ms2ger pushed a commit to web-platform-tests/wpt that referenced this issue May 20, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 19, 2019
…, a=testonly

Automatic update from web-platform-tests
Add some initial tests for JSON modules.

See <whatwg/html#4407>.

--

wp5At-commits: f510720b12c7d0dd3e0639bac35aceb237c2221c
wpt-pr: 16734
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jun 19, 2019
…, a=testonly

Automatic update from web-platform-tests
Add some initial tests for JSON modules.

See <whatwg/html#4407>.

--

wp5At-commits: f510720b12c7d0dd3e0639bac35aceb237c2221c
wpt-pr: 16734
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…, a=testonly

Automatic update from web-platform-tests
Add some initial tests for JSON modules.

See <whatwg/html#4407>.

--

wp5At-commits: f510720b12c7d0dd3e0639bac35aceb237c2221c
wpt-pr: 16734

UltraBlame original commit: d18767ce41b850b8929ad67e8367a5fb80eda67d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…, a=testonly

Automatic update from web-platform-tests
Add some initial tests for JSON modules.

See <whatwg/html#4407>.

--

wp5At-commits: f510720b12c7d0dd3e0639bac35aceb237c2221c
wpt-pr: 16734

UltraBlame original commit: d18767ce41b850b8929ad67e8367a5fb80eda67d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…, a=testonly

Automatic update from web-platform-tests
Add some initial tests for JSON modules.

See <whatwg/html#4407>.

--

wp5At-commits: f510720b12c7d0dd3e0639bac35aceb237c2221c
wpt-pr: 16734

UltraBlame original commit: d18767ce41b850b8929ad67e8367a5fb80eda67d
@littledan
Copy link
Contributor Author

@littledan littledan commented Nov 12, 2019

This patch has already been reverted, but I'm wondering if it'd be blocked by nosniff, since we never changed the destination to something that isn't script-like for JSON modules. (I don't even know if there is an appropriate destination type in fetch, or if we should add a new one.)

Loading

@annevk
Copy link
Member

@annevk annevk commented Nov 12, 2019

I think you're right that it would've been blocked. I'm surprised that didn't come up as implementation feedback.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants