-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Allow "style" and "json" in as attribute for modulepreload
#11981
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
base: main
Are you sure you want to change the base?
Allow "style" and "json" in as attribute for modulepreload
#11981
Conversation
as attributeas attribute in modulepreload
as attribute in modulepreloadas attribute for modulepreload
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 we also disallow "json" for <link rel=preload>?
You're right, "json" doesn't really make sense in preload if it's supported for Filed #11995 to track and discuss separately. |
|
I'd rather tackle them together as I have a feeling #11995 will not be addressed anytime soon otherwise. And it would be good to have that infrastructure in place as we add more types, such as |
|
I assumed there would be some window of overlap between the new and deprecated version, which is why I filed a separate bug. But if we can do them in one shot that's great. This feature is a little different in that it's not directly observable, so it does seem safe to just deprecate. I'll update this PR to remove @rniwa - you recently added support for |
|
Latest push handles removing "json" for "preload". I also added the implementation bugs. I'll work on tests next. |
|
Tests and MDN PR's have been are added, so the only missing part in compliance above is "two implementors are interested". @rniwa - can you give a WebKit position in https://bugs.webkit.org/show_bug.cgi?id=303761? |
|
@annevk - you mentioned a missing conformance section for the |
|
@KurtCattiSchmidt those are the conformance requirements. You'll have to adjust the wording there to make it clear
Or maybe define preload destination as a concept which takes care of that. |
|
Thanks @annevk - it was awkward updating "potential destination" to exclude "json" everywhere, so I added a preload destination concept. This reduced some duplication elsewhere too. |
source
Outdated
| @@ -16396,18 +16390,23 @@ interface <dfn interface>HTMLLinkElement</dfn> : <span>HTMLElement</span> { | |||
|
|
|||
| <hr> | |||
|
|
|||
| A <dfn data-x="concept-preload-destination">preload destination</dfn> is a <span | |||
| data-x="concept-potential-destination">potential destination</span>, excluding "<code | |||
| data-x="">json</code>" when used with <code data-x="rel-preload">preload</code> links, and is | |||
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.
Excluding a single thing this way is a bit icky special case, and special cases tend to cause issues later. Would there be some other way to express this (if we need to exclude json. Not sure why we need to, but perhaps it doesn't matter too much.).
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.
We might have to exclude more module destination types going forward. One way to avoid this would be to refactor Fetch's destination concept to have subtypes or some such. But not sure we need to go there right away.
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.
Added a "preload destination" concept in the latest push that instead lists all supported module types (in addition to the "module preload destination" concept added for module preloads).
It's already disallowed. See https://html.spec.whatwg.org/multipage/links.html#translate-a-preload-destination |
Not exactly, https://html.spec.whatwg.org/#consume-a-preloaded-resource lists JSON as a preload destination. I reverted to the prior commit in this PR, which removes that part and adds "style" and "json" to |
|
The latest push addresses everything discussed this morning (returned conformance section, listed all supported values as a "preload destination" concept). |
source
Outdated
| data-x="">audio</code>", "<code data-x="">fetch</code>", "<code data-x="">font</code>", "<code | ||
| data-x="">image</code>", "<code data-x="">style</code>", "<code data-x="">track</code>", "<code | ||
| data-x="">video</code>", or a <span | ||
| data-x="concept-script-like-destination">script-like destination</span>. |
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 is a change in behavior. Do we know if e.g. <link rel=preload as="sharedworker"> works or supposed to work?
I see that the test variants don't cover this.
Maybe keep it as script and we can check if it works for other script-like destinations separately?
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 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.
The "Consume a Preloaded Resource" section lists all script-like destinations as supported: https://html.spec.whatwg.org/#consume-a-preloaded-resource
(it's in step 6 of "For the purposes of this section, a string type matches a string destination if the following algorithm returns true:" under "If any of the following are true:...")
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 think limiting it to "script" would be a change in behavior given the above. But perhaps that is incorrect and it should be "script" there instead of "script-like destination". Note that Chrome logs an error that it's valid but unsupported with "sharedworker" (see the links here #11981 (comment)).
source
Outdated
| destination</span>. For other <code>link</code> elements, it must not be specified.</p> | ||
| have a value which is a <span | ||
| data-x="concept-module-preload-destination">module preload destination</span>. For other | ||
| <code>link</code> elements, it must not be 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.
I don't think this quite works. Initially when we introduce the attribute we should say it takes either a preload or module preload destination. Then we should say that each destination of the union of these destinations is a keyword, mapping to a state of the same name. Otherwise the enumerated attribute definition doesn't work.
Then we restrict per rel attribute value what keywords are valid.
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.
You're right, this is much clearer now. I moved "This is an enumerated attribute" to be before listing the valid values. It was a little awkward phrasing "the union of..." but I think the latest version is decent.
source
Outdated
| @@ -29009,13 +28985,16 @@ document.body.appendChild(wbr);</code></pre> | |||
| data-x="concept-request-credentials-mode">credentials mode</span>.</p> | |||
| </div> | |||
|
|
|||
| A <dfn data-x="concept-preload-destination">preload destination</dfn> is "<code | |||
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.
Ditto.
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.
Per above, I had to remove the "concept-" prefix for this to compile. This revealed that there's already a definition for "preload destination" that listed a string, which I changed to point to the full list of preload destinations.
annevk
left a comment
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 looks great @KurtCattiSchmidt, thanks for tidying it all up. @smaug---- @noamr any final thoughts?
I suggest we merge this Monday next week.
Re-read the PR, looks good to me! |
@noamr - would you mind hitting the +1 button again? You had previously approved, but it reset when I re-requested review. |
Thanks Anne! We have a holiday on Monday so I'll plan on landing on Tuesday unless any new issues arise. |
|
The editors will take care of the merging (and are available on Monday). Unless you wanted to push it back one day? Though it wouldn't give the US an extra day so then we'd have to merge Wednesday. |
|
If someone else is merging then Monday is fine with me, thanks for checking. |
Updates the definition of
modulepreloadto supportas="style"andas="json"for preloading style and JSON module scripts respectively. Also removesas=jsonforrel=preload, as the only use for preloading JSON is for a module import, so it might as well directly add it to the module graph viamodulepreload. There were some inconsistencies with the existing spec's supportedasvalues forrel=preload, which this PR fixes with the introduction of a "preload destination" concept, which includes all supported types.Fixes #10233
modulepreloadlinks with JSON and CSS, remove json with non-module preload web-platform-tests/wpt#56617modulepreloadandpreloadto incorporate WHATWG changes for "style" and "json" mdn/content#42333(See WHATWG Working Mode: Changes for more details.)
/acknowledgements.html ( diff )
/indices.html ( diff )
/links.html ( diff )
/semantics.html ( diff )