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.

Sign up for GitHub

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

Jump to bottom

fix: update modulePreload polyfill, dedupe same href fetch #12081

Closed
wants to merge 2 commits into from
Closed

fix: update modulePreload polyfill, dedupe same href fetch #12081

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ec61c7
fix: update modulePreload polyfill, dedupe same href fetch
patak-dev Feb 16, 2023
12a5255
chore: update copyright year
patak-dev Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 9 additions & 9 deletions packages/vite/src/node/plugins/modulePreloadPolyfill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function modulePreloadPolyfillPlugin(config: ResolvedConfig): Plugin {
The following polyfill function is meant to run in the browser and adapted from
https://github.com/guybedford/es-module-shims
MIT License
Copyright (C) 2018-2021 Guy Bedford
Copyright (C) 2018-2023 Guy Bedford
Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
Expand All @@ -62,10 +62,6 @@ function polyfill() {
return
}

for (const link of document.querySelectorAll('link[rel="modulepreload"]')) {
processPreload(link)
}

new MutationObserver((mutations: any) => {
for (const mutation of mutations) {
if (mutation.type !== 'childList') {
Expand All @@ -78,6 +74,10 @@ function polyfill() {
}
}).observe(document, { childList: true, subtree: true })

for (const link of document.querySelectorAll('link[rel="modulepreload"]')) {
processPreload(link)
}

function getFetchOpts(link: any) {
const fetchOpts = {} as any
if (link.integrity) fetchOpts.integrity = link.integrity
Expand All @@ -89,12 +89,12 @@ function polyfill() {
return fetchOpts
}

const fetchCache = {} as any
function processPreload(link: any) {
if (link.ep)
// ep marker = processed
return
if (link.ep) return
link.ep = true
// prepopulate the load record
if (fetchCache[link.href]) return
fetchCache[link.href] = true
Comment on lines +96 to +97
Copy link
Member

Choose a reason for hiding this comment

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

IIUC this part only makes sense with es-module-shims.
es-module-shims uses fetch to fetch the module and executes them with import(blobUrl).
https://github.com/guybedford/es-module-shims/blob/e8eb942fdd65de8669184a4c1d8ad7dde21ea576/src/es-module-shims.js#L297
https://github.com/guybedford/es-module-shims/blob/e8eb942fdd65de8669184a4c1d8ad7dde21ea576/src/es-module-shims.js#L233

Because we don't fetch modules on our side and rely on the browsers mechanism, fetchCache won't be used when the modules are fetched.
So this fetchCache[link.href] part does the same thing with link.ep.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I missed this one. I didn't even look for the cache as I assumed it was local because I thought module preload polyfill should work with regular imports. Is your understanding of the polyfill that we can't extract the module preload and we should use the whole thing? The polyfill isn't small and IMO we shouldn't mess with the way modules are imported to support module preload. Not by default, and users could add this polyfill themselves if they want that.

But I think fetchCache was added to fix an edge case, and it was working in the general case without it (even if there is a double request).

So, we should close this PR. Then find out if the polyfill is really working. I need some help evaluating this, I thought it was working but I'm not sure anymore now.

Maybe for 4.2 we should get back to the original use of 'preload' in browsers that don't support 'modulepreload'. See cb75dbd#diff-121079017d1fa98d6a0ea3354d5c73df45ab277078228cd7384c7a4e84c5a813L35.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree too. Looks like fetchCache doesn't have a benefit here. I think reverting to using preload would be the best bet for now too if modulepreload doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Is your understanding of the polyfill that we can't extract the module preload and we should use the whole thing?

I'm not sure about this. But at least this fetchCache part cannot be extracted. If this fetchCache mechanism is necessary to make the polyfill work, we can't extract only this part.

The polyfill isn't small and IMO we shouldn't mess with the way modules are imported to support module preload.

I agree to this.

Then find out if the polyfill is really working.

When I added this comment (#5532 (comment)), firefox did work when Cache-Control was set. I don't know whether it works now though.

const fetchOpts = getFetchOpts(link)
fetch(link.href, fetchOpts)
}
Expand Down