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

Add <link> rel="modulepreload" #2383

Merged
merged 3 commits into from Dec 14, 2017
Merged

Add <link> rel="modulepreload" #2383

merged 3 commits into from Dec 14, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Feb 22, 2017

This allows preloading module script graphs. The processing model for
this turns out to be different enough that simply extending
rel="preload" is not a good option. Summary of why:

whatwg/fetch#486 (comment)

Closes whatwg/fetch#486.

Preview: https://output-kxakhvahwc.now.sh/multipage/links.html#link-type-modulepreload


/cc @whatwg/modules @yoavweiss


/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/links.html ( diff )
/scripting.html ( diff )
/semantics.html ( diff )
/urls-and-fetching.html ( diff )
/webappapis.html ( diff )

@annevk
Copy link
Member

annevk commented Feb 23, 2017

If I have a document A with a same-origin <iframe> B. I think with a rel=preload in A I can get the resource cached for A and B. (Although this is somewhat unclear due to it not being defined...) With the setup you propose here you would get a new load for B. Is that what we want?

source Outdated

<li><p><span data-x="parse a url">Parse</span> the <span>URL</span> given by the <code
data-x="attr-link-href">href</code> attribute, relative to the element's <span>node
document</span>. If that fails, then abort these steps. Otherwise, let <var>url</var> be the
Copy link
Member

Choose a reason for hiding this comment

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

Fire error here if w3c/preload#88 is so changed.

@@ -12307,6 +12329,7 @@ interface <dfn>HTMLLinkElement</dfn> : <span>HTMLElement</span> {
<code data-x="rel-alternate">alternate</code>,
<code data-x="rel-dns-prefetch">dns-prefetch</code>,
<code data-x="rel-icon">icon</code>,
<code data-x="rel-modulepreload">modulepreload</code>,
Copy link
Member

Choose a reason for hiding this comment

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

Can we call it preloadmodule instead, so it's listed next to preload and autocomplete works better?

Copy link
Member Author

Choose a reason for hiding this comment

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

That does seem reasonable. Will update and rename the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought... "preloadmodule" sounds like "preload a module" ("why not just use preload??"), or maybe "a module specifying a bunch of preload stuff".

Whereas "modulepreload" sounds like "a module-specific variant of preload".

Seems better as-is to me now...

@domenic
Copy link
Member Author

domenic commented Feb 23, 2017

If I have a document A with a same-origin <iframe> B. I think with a rel=preload in A I can get the resource cached for A and B. (Although this is somewhat unclear due to it not being defined...) With the setup you propose here you would get a new load for B. Is that what we want?

I didn't know that was true; I guess that falls into the same "preload cache is unspecified" bucket. But yes, this is definitely what we want; this is a great point as to why it has to be separate. Each document has a separate module map, and we don't want to introduce ways for one document to accidentally start overwriting the other document's module map.

@domenic domenic added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Feb 24, 2017
source Outdated

<div w-nodev>

<p>The following contains are the appropriate times to fetch the resource for such a link:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

<p>The following contains are the appropriate times to fetch the resource for such a link:</p>

Drop the word contains here?

source Outdated
<!-- integrity?? https://github.com/whatwg/html/issues/2382 -->

<li><p><span data-x="concept-fetch">Fetch</span> <var>request</var>, and asynchronously wait
to run the remaining steps as part of fetch's <span>process response</span> for the <span
Copy link
Contributor

Choose a reason for hiding this comment

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

  to run the remaining steps as part of fetch's <span>process response</span> for the <span 

Add the before fetch here? So, as part of the fetch's process response

@domenic
Copy link
Member Author

domenic commented Jul 24, 2017

Note to future self if this garners enough implementer interest to be picked back up: we can actually instantiate during preloading as well. The spec should be updated to do so.

@snuggs
Copy link
Member

snuggs commented Jul 24, 2017

@domenic have seen you mention "implementor interest" before. Is that the vendor "implementor" library "implementor" or both. Thanks for the clarity my man! 🙏

@domenic
Copy link
Member Author

domenic commented Jul 24, 2017

Browsers are the ones who implement the spec.

domenic added a commit that referenced this pull request Sep 14, 2017
The primary normative content of this commit is that it fixes #2382 by
passing the integrity metadata to the fetch call for the top-level
module script, in <script type=module>.

However, the way it does this is via a larger refactoring, which is
setting the stage for #2315. It creates a new struct, the script fetch
options, which is now shared by both module and classic scripts. Storing
this for classic scripts is not currently useful, but will be for #2315
when, via import(), classic scripts are able to import module scripts,
and need these fetch options to do so.

This will also be useful for when we revive #2383, as
<link rel=modulepreload> can have referrerpolicy="" specified on it,
which will need to be passed down. (It would also be useful if we ever
do w3c/webappsec-referrer-policy#96 and add
referrerpolicy="" to <script>.) With this structure in place, it's a
simple matter of adding a referrer policy item to the script fetch
options.
domenic added a commit that referenced this pull request Oct 3, 2017
The primary normative content of this commit is that it fixes #2382 by
passing the integrity metadata to the fetch call for the top-level
module script, in <script type=module>.

However, the way it does this is via a larger refactoring, which is
setting the stage for #2315. It creates a new struct, the script fetch
options, which is now shared by both module and classic scripts. Storing
this for classic scripts is not currently useful, but will be for #2315
when, via import(), classic scripts are able to import module scripts,
and need these fetch options to do so.

This will also be useful for when we revive #2383, as
<link rel=modulepreload> can have referrerpolicy="" specified on it,
which will need to be passed down. (It would also be useful if we ever
do w3c/webappsec-referrer-policy#96 and add
referrerpolicy="" to <script>.) With this structure in place, it's a
simple matter of adding a referrer policy item to the script fetch
options.
domenic added a commit that referenced this pull request Oct 3, 2017
The primary normative content of this commit is that it fixes #2382 by
passing the integrity metadata to the fetch call for the top-level
module script, in <script type=module>.

However, the way it does this is via a larger refactoring, which is
setting the stage for #2315. It creates a new struct, the script fetch
options, which is now shared by both module and classic scripts. Storing
this for classic scripts is not currently useful, but will be for #2315
when, via import(), classic scripts are able to import module scripts,
and need these fetch options to do so.

This will also be useful for when we revive #2383, as
<link rel=modulepreload> can have referrerpolicy="" specified on it,
which will need to be passed down. (It would also be useful if we ever
do w3c/webappsec-referrer-policy#96 and add
referrerpolicy="" to <script>.) With this structure in place, it's a
simple matter of adding a referrer policy item to the script fetch
options.
@nyaxt
Copy link
Member

nyaxt commented Oct 27, 2017

For Chromium/Blink, @irori have ready to land implementation at https://chromium-review.googlesource.com/c/chromium/src/+/662697

@guybedford
Copy link

Is there a separate spec for rel=preload as applied to modules, or is that determined to not be a specified feature?

As far as I can tell, the main use case this would be suitable for seems to be in preloading not-yet-navigated features, while saving top-level main thread execution costs?

If I want to preload large unbundled dependency graphs, then I want to preload every module that is not at the top-level to avoid dependency detection latency, which for a graph of 1000 modules (n) might be say 998 modules (that is, O(n)). Now if each one of my preloads is in turn running a full depgraph preload over its subgraph, that means I might be running of the order of O(n^2/2) graph traversals, which will be a performance issue (~ half a million for 1000 module case!). With flat hinting, only a single traversal of O(n) would be needed, which I'm still really interested to see clearly provided as a separate path for preload hinting for dependency graphs, separate to the depgraph traversal.

@domenic
Copy link
Member Author

domenic commented Oct 27, 2017

rel=preload as applied to modules is already specified by existing web specifications. It doesn't work very well, for the reasons gone over many times already, thus this new spec.

It's easily possible to avoid the O(n^2) problem. As you're probably aware, ES modules themselves involve graph traversals and subgraph traversals and they aren't O(n^2).

@guybedford
Copy link

guybedford commented Oct 27, 2017

We had that discussion yes, but I mentioned that single hints are separate to graph hints. These are two use cases that both need to be served. Can you please confirm that the non-traversing hint case is not being supported in spec currently?

A single traversal is O(n) as we have stop conditions on visited. Traversing every module as a top-level preload can be O(n^2/2) unless this algorithm were to be very cleverly implemented with each unique top-level preload visitation sharing a visitation cache, while maintaining circular reference orderings. I doubt any implementation would do anything more than the naive traversal though, thus giving us an O(n^2) preloading worst case for a platform feature, when applied to this hinting use case of preloading every module at depth > 0.

@snuggs
Copy link
Member

snuggs commented Oct 27, 2017

@domenic not to bikeshed but wouldn't rel=module be Ockham's razor? Although there may be good reasons i'm unaware of that we need two words. Perhaps answer somewhere in whatwg/fetch#486 and will read now.

Secondly where is the mutual exclusion nomodule boolean attr come in to play with this strategy and would that be an allowable attribute? Coincidentally enough I we've thinking of this for the past few days.

Thanks in advance!

/cc @brandondees

@domenic
Copy link
Member Author

domenic commented Oct 27, 2017

@guybedford I feel like we are going in circles, so I'm not sure how to proceed. I can't keep repeating myself here.

@snuggs the OP links to reasons why we need two separate rels.

@guybedford
Copy link

@domenic there are no circles to be found here apart from those of cyclic dependency graphs :P I'd really appreciate it if you could just answer my two simple questions, which haven't been stated elsewhere, then I've had my say:

  1. Can you confirm that this is the only preload for modules being specified, and there is not a preload being specified that does not automatically attempt to follow dependency preloading.

  2. Do you expect implementors to optimize foreach( module in graph ) { modulepreload(module) } (created by a separate <link rel=modulepreload> for each module in the graph) such that it is O(n) and not O(n^2) where n is the size of the graph, while supporting circular reference tracking?

Will be much appreciated.

@domenic
Copy link
Member Author

domenic commented Oct 27, 2017

  1. There are too many negatives in that sentence for me to understand it.

  2. Yes, easily; this is already what they have to do for multiple <script type=module> elements.

@guybedford
Copy link

Sure, thanks, I'll run the benchmarks with real trees using this flat preloading optimization method in due course. I will trust your judgement here, but I sincerely hope these algorithmic requirements don't stunt the performance of preloading 1000s of modules in a tree simultaneously.

@snuggs
Copy link
Member

snuggs commented Nov 2, 2017

@domenic can we get a rebase please? 🙏

@domenic
Copy link
Member Author

domenic commented Nov 2, 2017

I will work on this when I have time; "bump" comments are not really helpful.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 6, 2017
This implements link rel=modulepreload without submodules fetch.

LinkLoader fetches module script using Modulator::FetchSingle, so it
populates the module map but does not fetch descendant modules.
Unlike the preload fetches by link rel=preload, this doesn't set
FetchParameters::SetLinkPreload flag, so it works like a regular
script fetch.

HTMLPreloadScanner fetches module scripts in a similar way to
speculative preload of <script type=module>, i.e. just fetch a
ScriptResource and not populate the module map.

Spec PR: whatwg/html#2383

Change-Id: I5f4420e305f4962b3fbe3f703ce95a5a43e4f7a9
@domenic
Copy link
Member Author

domenic commented Nov 7, 2017

All updated, including making fetching dependencies optional but encouraged. https://output-kxakhvahwc.now.sh/multipage/links.html#link-type-modulepreload has a preview.

@domenic
Copy link
Member Author

domenic commented Nov 10, 2017

Shoot, this neglects referrerpolicy="" on the link element. That's going to need a bit more work. Note to self to fix when I have some time.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 10, 2017
This implements link rel=modulepreload without submodules fetch,
behind the Experimental Web Platform feature flag.

LinkLoader fetches module script using Modulator::FetchSingle, so it
populates the module map but does not fetch descendant modules.
Unlike the preload fetches by link rel=preload, this doesn't set
FetchParameters::SetLinkPreload flag, so it works like a regular
script fetch.

HTMLPreloadScanner fetches module scripts in a similar way to
speculative preload of <script type=module>, i.e. just fetch a
ScriptResource and not populate the module map.

Spec PR: whatwg/html#2383
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/ynkrM70KDD4

Bug: 740886
Change-Id: I5f4420e305f4962b3fbe3f703ce95a5a43e4f7a9
Reviewed-on: https://chromium-review.googlesource.com/662697
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515490}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 10, 2017
This implements link rel=modulepreload without submodules fetch,
behind the Experimental Web Platform feature flag.

LinkLoader fetches module script using Modulator::FetchSingle, so it
populates the module map but does not fetch descendant modules.
Unlike the preload fetches by link rel=preload, this doesn't set
FetchParameters::SetLinkPreload flag, so it works like a regular
script fetch.

HTMLPreloadScanner fetches module scripts in a similar way to
speculative preload of <script type=module>, i.e. just fetch a
ScriptResource and not populate the module map.

Spec PR: whatwg/html#2383
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/ynkrM70KDD4

Bug: 740886
Change-Id: I5f4420e305f4962b3fbe3f703ce95a5a43e4f7a9
Reviewed-on: https://chromium-review.googlesource.com/662697
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515490}
MXEBot pushed a commit to mirror/chromium that referenced this pull request Nov 11, 2017
This implements link rel=modulepreload without submodules fetch,
behind the Experimental Web Platform feature flag.

LinkLoader fetches module script using Modulator::FetchSingle, so it
populates the module map but does not fetch descendant modules.
Unlike the preload fetches by link rel=preload, this doesn't set
FetchParameters::SetLinkPreload flag, so it works like a regular
script fetch.

HTMLPreloadScanner fetches module scripts in a similar way to
speculative preload of <script type=module>, i.e. just fetch a
ScriptResource and not populate the module map.

Spec PR: whatwg/html#2383
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/ynkrM70KDD4

Bug: 740886
Change-Id: I5f4420e305f4962b3fbe3f703ce95a5a43e4f7a9
Reviewed-on: https://chromium-review.googlesource.com/662697
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515490}
@Constellation
Copy link
Member

r=me. WebKit side bug is https://bugs.webkit.org/show_bug.cgi?id=180574.

@annevk
Copy link
Member

annevk commented Dec 8, 2017

@Constellation thanks for confirming interest from WebKit and reviewing the PR. Does WebKit not yet implement referrer policy so that's not a concern?

@domenic
Copy link
Member Author

domenic commented Dec 8, 2017

I think it's just that fixing the referrerpolicy="" thing is pretty easy and obvious. I will try to have it done today anyway.

This allows preloading module scripts, and optionally their descendants.
The processing model for this turns out to be different enough that
simply extending rel="preload" is not a good option.

Closes whatwg/fetch#486.
@domenic domenic removed the needs implementer interest Moving the issue forward requires implementers to express interest label Dec 8, 2017
@domenic
Copy link
Member Author

domenic commented Dec 8, 2017

referrerpolicy="" is now integrated. @annevk, would you mind doing an editorial review when you have time?

Tests are supposed to land with Chromium, but we should be sure to audit them a bit before merging.

@domenic domenic added the needs tests Moving the issue forward requires someone to write tests label Dec 8, 2017
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Incorrect as="" results in an error but incorrect href="" does not. Seems a little weird, but consistently weird perhaps?

Looks good apart from the referrer policy thing.

data-x="concept-script-fetch-options-credentials">credentials mode</span> is "<code
data-x="">omit</code>".</p>
data-x="">omit</code>", and <span data-x="concept-script-fetch-options-referrer-policy">referrer
policy</span> is the empty string.</p>
Copy link
Member

Choose a reason for hiding this comment

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

This looks a little wrong. Why would it always be the empty string?

Copy link
Member Author

Choose a reason for hiding this comment

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

These default options are used for javascript: URLs, classic worker scripts, importScripts(), and setTimeout evaluation, wherein there is no way to set the referrer policy. Setting it to the empty string makes Fetch infer it from the client, which seems correct in these cases. Did I get that right?

Copy link
Member

Choose a reason for hiding this comment

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

I think that at least worker scripts should have a referrer policy since they create a global. It seems reasonable for the others. (I was somewhat wrong here as I assumed this would mean the equivalent of omit referrer.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for fetching the script itself though. Right now that fetch just inherits from the client; are you thinking of introducing a normative change?

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, in that case it's all good I think. Thanks for helping me out.

@annevk
Copy link
Member

annevk commented Dec 9, 2017

Oh also, it seems this'll need implementation bugs for Edge and Firefox.

@kaizhu256
Copy link

playing devil's advocate, but how is all this complexity and trouble better than the simpler alternative of using a rollup script? to some, this looks like alot of effort to solve a problem that shouldn't exist as long as people continue to use rollups.

@annevk
Copy link
Member

annevk commented Dec 13, 2017

@kaizhu256 the idea behind the platform adopting infrastructure pioneered by libraries and frameworks is that such libraries and frameworks then get to focus on the next level of abstraction and not have to worry about this anymore.

@domenic
Copy link
Member Author

domenic commented Dec 14, 2017

I found the tests, and they look good: https://github.com/w3c/web-platform-tests/blob/master/preload/modulepreload.html

Just waiting to make sure I addressed all of @annevk's feedback at this point :). Then I'll merge and file bugs.

@domenic domenic removed the needs tests Moving the issue forward requires someone to write tests label Dec 14, 2017
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

FWIW, the potential issue with worker scripts shouldn't block this I think, but please file a follow-up if I'm correct.

Also please file implementation bugs. I don't think I missed a comment for that this time around.

@domenic domenic merged commit 7c28027 into master Dec 14, 2017
@domenic domenic deleted the modulepreload branch December 14, 2017 20:04
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
The primary normative content of this commit is that it fixes whatwg#2382 by
passing the integrity metadata to the fetch call for the top-level
module script, in <script type=module>.

However, the way it does this is via a larger refactoring, which is
setting the stage for whatwg#2315. It creates a new struct, the script fetch
options, which is now shared by both module and classic scripts. Storing
this for classic scripts is not currently useful, but will be for whatwg#2315
when, via import(), classic scripts are able to import module scripts,
and need these fetch options to do so.

This will also be useful for when we revive whatwg#2383, as
<link rel=modulepreload> can have referrerpolicy="" specified on it,
which will need to be passed down. (It would also be useful if we ever
do w3c/webappsec-referrer-policy#96 and add
referrerpolicy="" to <script>.) With this structure in place, it's a
simple matter of adding a referrer policy item to the script fetch
options.
@simeyla
Copy link

simeyla commented Feb 11, 2020

...and after all that and three years later 'modulepreload' still doesn't work on Cloudflare :-(

@zcorpan
Copy link
Member

zcorpan commented Feb 12, 2020

@simeyla this is not the right place to report issues with Cloudflare. Try https://support.cloudflare.com/hc/en-us/articles/200172476-Contacting-Cloudflare-Support or https://community.cloudflare.com/

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

Successfully merging this pull request may close these issues.

preload, destinations, and module scripts
10 participants