Modulepreload error caching #273
Comments
|
The intended/specified behavior is that, like 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 |
|
The bug then seems to be that this works in the uncached case - that is, |
|
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? |
|
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). |
|
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. |
|
@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? |
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.)
Yep, sad but true.
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.
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. |
|
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! |
|
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. |
…headers (dunglas) This PR was merged into the 6.3 branch. Discussion ---------- [FrameworkBundle] remove support for preloading ESM using headers | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | n/a | License | MIT | Doc PR | n/a Sorry to be that late on this one. It looks like preloading ESM using HTTP headers (and so using 103 Early Hints) doesn't work, at least for now: WICG/import-maps#273 (comment) ImportMap must be defined before downloading any ESM, and by definition HTTP headers are sent before importmap definition (it's currently not possible to define importmap using headers). I don't know why I didn't caught this during my previous testing session, but now I get this error in the console when using this feature (which, according to the previously mentioned issue, is expected): ``` An import map is added after module script load was triggered. Uncaught TypeError: Failed to resolve module specifier "app". Relative references must start with either "/", "./", or "../". ``` This patch simply removes this feature, which is entirely non-functional and misleading for the end user. Commits ------- 7776c28 [FrameworkBundle] remove support for preloading ESM modules using headers
@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.jsuses 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:
<head>processing requirement or some other kind of phase handling.The text was updated successfully, but these errors were encountered: