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

Preload: only allow certain values for the as attribute #10212

Merged
merged 4 commits into from
Apr 1, 2024

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Mar 19, 2024

Closes #8332

(See WHATWG Working Mode: Changes for more details.)


/links.html ( diff )
/semantics.html ( diff )

source Outdated Show resolved Hide resolved
@noamr noamr requested a review from annevk March 19, 2024 16:07
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Looks good with nits. Although I wonder what the deal is with json and whether adding that to the spec was a mistake since nobody seems to be working on implementing it?

source Show resolved Hide resolved
source Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Mar 26, 2024

Looks good with nits. Although I wonder what the deal is with json and whether adding that to the spec was a mistake since nobody seems to be working on implementing it?

This seems like "somebody is working on implementing it", no?

@tunetheweb
Copy link
Contributor

I wonder if we should leave JSON off until implementers implement that? Just having it as a destination does not mean it works for preload (the whole point of this PR is to make that clear!) so I'm not sure that as=json is currently different to any of the other valid destination values that don't currently work for preload?

@noamr
Copy link
Contributor Author

noamr commented Mar 26, 2024

I wonder if we should leave JSON off until implementers implement that? Just having it as a destination does not mean it works for preload (the whole point of this PR is to make that clear!) so I'm not sure that as=json is currently different to any of the other valid destination values that don't currently work for preload?

It's implemented and tested, at least in part: web-platform-tests/wpt#41665

@tunetheweb
Copy link
Contributor

I beg to differ:

https://wpt.fyi/results/preload?label=experimental&label=master&aligned

image

https://wpt.fyi/results/preload/supported-as-values.html%3Fas%3Djson%26expected%3D1?label=experimental&label=master&aligned

But I do agree having WPT is a good first start so seems the intent is to eventually support this?

@domenic
Copy link
Member

domenic commented Mar 27, 2024

This seems like "somebody is working on implementing it", no?

I can't tell. It says the issue is fixed, so I anticipate no more work will happen on it. But, the WPT still doesn't pass. Maybe @nicolo-ribaudo can clarify?

@noamr
Copy link
Contributor Author

noamr commented Mar 27, 2024

Removed as=json from the spec for the time being, we can add it when it's implemented anywhere.

@nicolo-ribaudo
Copy link
Contributor

I originally added as=json just because all destinations were exposed (#9486 (comment)). Then I didn't implement in Chromium it while working on https://issues.chromium.org/issues/40285036 just because... I forgot 😅

The use case for JSON is that you might want to pre-download your full JS modules graph (i.e. same use case as rel="script"), however JSON modules are always leaves of the graph so they don't cause waterfalls: not preloading them is not as bad as not preloading JS modules.

If we want to support it, we should also make sure that <link rel=modulepreload as=json> works. I can open an issue to discuss it, but I don't have a preference regarding keeping rel=preload as=json or not until when we reach a conclusion on that.

@noamr
Copy link
Contributor Author

noamr commented Mar 28, 2024

I originally added as=json just because all destinations were exposed (#9486 (comment)). Then I didn't implement in Chromium it while working on https://issues.chromium.org/issues/40285036 just because... I forgot 😅

The use case for JSON is that you might want to pre-download your full JS modules graph (i.e. same use case as rel="script"), however JSON modules are always leaves of the graph so they don't cause waterfalls: not preloading them is not as bad as not preloading JS modules.

If we want to support it, we should also make sure that <link rel=modulepreload as=json> works. I can open an issue to discuss it, but I don't have a preference regarding keeping rel=preload as=json or not until when we reach a conclusion on that.

OK, thinking about this again, since JSON is a module-thing, it only makes sense when preloading a module graph (modulepreload). Since modulepreload anyway doesn't have as, this should be done automatically when parsing the module that uses the JSON. In either case, having <link rel=preload as=json> doesn't add much. I removed it from the spec and will remove from WPT.

@tunetheweb
Copy link
Contributor

Can we leave it in WPT (it's helpful to know what's NOT supported - and would have saved me writing a test case if I'd thought to look there) but change it from expected=1 to expected=0 like the other ones that are not supported?

@nicolo-ribaudo
Copy link
Contributor

OK, thinking about this again, since JSON is a module-thing, it only makes sense when preloading a module graph (modulepreload).
Since modulepreload anyway doesn't have as, this should be done automatically when parsing the module that uses the JSON

modulepreload has as (see step 3 of https://html.spec.whatwg.org/#link-type-modulepreload), and actually by reading that algorithm I think currently <link rel=modulepreload as=json> is specified to pre-load the JSON module and add it to the import map (even if it was not my intention when adding the JSON destination, because I didn't think about it).

Also, modulepreload doesn't guarantee that the dependencies of a module will be preloaded too, it's a potential transparent optimization that browsers can implement. The recommendation is to explicitly list all the dependencies in separate link rel=modulepreload tags.

@noamr
Copy link
Contributor Author

noamr commented Mar 28, 2024

OK, thinking about this again, since JSON is a module-thing, it only makes sense when preloading a module graph (modulepreload).

Since modulepreload anyway doesn't have as, this should be done automatically when parsing the module that uses the JSON

modulepreload has as (see step 3 of https://html.spec.whatwg.org/#link-type-modulepreload), and actually by reading that algorithm I think currently <link rel=modulepreload as=json> is specified to pre-load the JSON module and add it to the import map (even if it was not my intention when adding the JSON destination, because I didn't think about it).

Also, modulepreload doesn't guarantee that the dependencies of a module will be preloaded too, it's a potential transparent optimization that browsers can implement. The recommendation is to explicitly list all the dependencies in separate link rel=modulepreload tags.

Gotcha, makes sense. Anyway, all of that is beyond the scope of this patch.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Spec text LGTM. Let me know when the tests are updated for JSON.

@noamr
Copy link
Contributor Author

noamr commented Mar 29, 2024

Spec text LGTM. Let me know when the tests are updated for JSON.

Done (web-platform-tests/wpt#45426)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

<link rel=preload> as attribute supports less than potential destination
4 participants