search

WICG / import-maps

How to control the behavior of JavaScript imports
https://html.spec.whatwg.org/multipage/webappapis.html#import-maps
Other
2.65k stars 69 forks source link

Modulepreload error caching #273

Closed guybedford closed 2 years ago

guybedford commented 2 years ago

@lewisl9029 alerted me to the issue he found in https://bugs.chromium.org/p/chromium/issues/detail?id=1313317.

It seems that if there is a <link rel="modulepreload" href="/app.js" />, where /app.js uses bare specifiers, and the preload comes before the import map, then when caching is used it is possible for the preload to complete before the import map has been processed resulting in the bare specifier error being cached.

As a result, when one later imports <script type="module" src="/app.js">, the cached bare specifier error stops the module load from processing instead of returning the correct module.

Seems like it would be worth ironing this one out - questions:

domenic commented 2 years ago

The intended/specified behavior is that, like <script type="module" src="/app.js"></script>, such a <link> will cause "acquiring import maps" to flip to false, and thus prevent any import maps from being loaded. See https://wicg.github.io/import-maps/#integration-wait-for-import-maps

The upshot here, is that import maps should be before any modulepreloads. Allowing more flexibility here would require something similar to solving #248, e.g. https://github.com/guybedford/import-maps-extensions#lazy-loading-of-import-maps

guybedford commented 2 years ago

The bug then seems to be that this works in the uncached case - that is, <link rel="modulepreload"> is not currently flipping the "acquiring import maps" state. Is it worth updating the bug to track this as an implementation bug?

domenic commented 2 years ago

Hmm, so I guess I'm not 100% clear on which cases were tested.

This is the case I understand you to be describing:

Is there another case under discussion?

guybedford commented 2 years ago

The issue is that the import map is still applying after the preload in the uncached case, resulting in no bare specifier error.

Actual behaviour is that the import map is never blocked - so the application behaves differently depending on which wins - the modulepreload or the actual module tag, which then differs between cached and uncached loads.

The fix should be to ensure that modulepreload does flip the state (which it isn't right now).

domenic commented 2 years ago

Got it, yeah, then we should make sure that is tracked on the implementation bug and is part of the fix @hiroshige-g is working on there.

lewisl9029 commented 2 years ago

@guybedford @domenic thanks for the discussion and references!

I didn't take into account that as per the spec, modulepreload is supposed to be able to fetch the entire module graph, not just the linked module itself. So this behavior is starting to make more sense.

Interestingly enough, I initially ran into this issue when attempting to use modulepreload link headers, as opposed to elements, to allow browsers to discover them as early as possible, and in anticipation for Cloudflare's 103 early hints support.

Import maps can only be specified inside the document in the current implementation, and es-module-shim isn't going to be able to polyfill header loading behavior, so it seems impossible to use modulepreload link headers in combination with import maps today.

Is this something that might be supported in the future when external import maps gets implemented? So we can add a link preload header for the import map before all the modulepreload headers?

As a separate discussion, do we feel the behavior for modulepreload to fetch the entire module graph is actually useful enough to block this use case with modulepreload headers + inlined import maps? modulepreload won't be able to flatten module loading waterfalls on its own, and it's meant to be used for modules that will be imported almost immediately, triggering the full module graph load anyways, so seems to be of very limited value. I suspect this might also have something to do with why it hasn't been implemented yet?

domenic commented 2 years ago

I didn't take into account that as per the spec, modulepreload is supposed to be able to fetch the entire module graph, not just the linked module itself. So this behavior is starting to make more sense.

It's not just that. It's that modulepreload is supposed to parse and compile the module, which requires doing import specifier resolution. (Even if it doesn't do module fetching.)

Import maps can only be specified inside the document in the current implementation, and es-module-shim isn't going to be able to polyfill header loading behavior, so it seems impossible to use modulepreload link headers in combination with import maps today.

Yep, sad but true.

Is this something that might be supported in the future when external import maps gets implemented? So we can add a link preload header for the import map before all the modulepreload headers?

Interesting idea. It would need to be special-cased, but I've learned that we have similar special-cases for CSP in early hints already, so it's not impossible.

As a separate discussion, do we feel the behavior for modulepreload to fetch the entire module graph is actually useful enough to block this use case with modulepreload headers + inlined import maps?

I have always felt it's useful because browsers which will fetch at least the dependent modules will be strictly faster than browsers which don't, i.e. browsers which wait until actual module execution (which could be much later).

No browser has yet prioritized this aspect of making module loading faster though. Perhaps because they'd prefer authors to do the work of flattening the module graph.

lewisl9029 commented 2 years ago

Ah, understood. Seems like the best we can do today is put modulepreload link elements in our documents, which is good enough to at least flatten the waterfall. This was the biggest blocker for using unbundled modules in prod for me in the past, and I've already been really happy with the results!

In the future it'd be amazing to be able to leverage external import maps, modulepreload headers and early hints to squeeze out even more performance (and improve cache granularity, since the import map is usually the only thing that changes in my html), but we're definitely getting into diminishing returns here.

Still, super excited about the future, and thank you all for the work on this stuff so far!

domenic commented 2 years ago

I believe the spec here does not need changes, and the Chromium bug was fixed including with web platform tests. So I will close this now. Let me know if I missed something and there's a reason to keep it open.