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

feat: modulepreload polyfill #4058

Merged
merged 4 commits into from
Jul 29, 2021
Merged

feat: modulepreload polyfill #4058

merged 4 commits into from
Jul 29, 2021

Conversation

patak-dev
Copy link
Member

Description

Implement rel="modulepreload" polyfill as described in https://guybedford.com/es-module-preloading-integrity as proposed by @yyx990803

I had to modify importAnalysisBuild to always use 'modulepreload' if config.build.polyfillModulePreload is enabled, instead of the runtime detection

About naming, I used Used config.build.polyfillModulePreload and id vite:modulepreload-polyfill

Another option could be to use config.build.polyfillModulepreload and for the id vite:module-preload-polyfill

IIUC when modulepreload is supported (as in chrome), the polyfill will still issue a fetch. I trust Guy Beford here that this isn't a problem.

The polyfill was adapted from https://github.com/guybedford/es-module-shims/blob/main/src/es-module-shims.js, removing the import maps logic that was intervened with this polyfill (leaving almost the same version as in the blog post)

Tested in Firefox and seems to be fetching the scripts correctly, more testing would be good.

Additional Notes

We could wait after the beta is out to merge this one, or we could use the beta to test it so it is already part of 2.4.0


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jun 30, 2021
antfu
antfu previously approved these changes Jul 1, 2021
@patak-dev
Copy link
Member Author

I don't see a reason for always executing the polyfill, even for browsers that support modulepreload. Looks like in the polyfill this is what is being done https://github.com/guybedford/es-module-shims/blob/main/src/es-module-shims.js#L318

But we already have support detection for modulepreload in importAnalysisBuild

const relList = document.createElement('link').relList
const supportsModulePreload = relList && relList.supports && relList.supports('modulepreload')

I added a guard to the polyfill so we don't change things for chromium where this is already supported.

@patak-dev patak-dev added this to the 2.5 milestone Jul 9, 2021
@patak-dev
Copy link
Member Author

We discussed with the team that this feature is going to be merged before the beta for the next minor (2.5)

Shinigami92
Shinigami92 previously approved these changes Jul 27, 2021
docs/config/index.md Outdated Show resolved Hide resolved
Co-authored-by: Shinigami <chrissi92@hotmail.de>
@patak-dev patak-dev merged commit cb75dbd into vitejs:main Jul 29, 2021
@patak-dev patak-dev deleted the feat/module-preload-polyfill branch July 29, 2021 05:17
@croban
Copy link

croban commented Sep 13, 2021

@patak-js ist it possible to also to check polyfillModulePreload where skip const is defined e.g.
const skip = config.build.ssr || !config.build.polyfillModulePreload
otherwise it's looks like polyfill can't be removed?

@patak-dev
Copy link
Member Author

@croban could you raise a bug report with a reproduction if you found a problem? AFAICS the polyfill plugin is not included in config.build.polyfillModulePreload is false here

config.build.polyfillModulePreload

@derekyle
Copy link

Did this never make it into 2.4.0 or later? I am currently running vite 2.6.7 and I see the modulepreload-polyfill script getting loaded in firefox, but I am also seeing some files downloaded twice in Firefox. Interestingly, the modulepreload-polyfill is being loaded using rel=modulepreload and is one of the files being downloaded twice.

@patak-dev
Copy link
Member Author

Would you open an issue with a reproduction so we can better track this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chunks reporting a preload link warning in Firefox
5 participants