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

Issue with <link rel="preload"> #111

Closed
josephliccini opened this issue Jun 17, 2019 · 3 comments · Fixed by #118
Closed

Issue with <link rel="preload"> #111

josephliccini opened this issue Jun 17, 2019 · 3 comments · Fixed by #118

Comments

@josephliccini
Copy link

Hi!

Thanks for this plugin!

It looks like Chrome (probably other browsers too) have issues with the combination of SRI and <link rel="preload">

You might want to put in the notes that this, combined with /* webpackPreload: "true" */ of lazy chunks will break the preloading mechanism until <link rel="preload"> also supports SRI:

w3c/preload#127

For us, that means disabling this plugin for now, since we need to be able to preload the lazy chunks for performance reasons as to not have a double-fetch for a resource.

@josephliccini
Copy link
Author

josephliccini commented Jun 17, 2019

I did some testing, and it looks like Firefox, Safari, and Edge are fine, just Chrome has this issue.

@jscheid
Copy link
Collaborator

jscheid commented Jun 19, 2019

Hi, thanks for bringing this to my attention. I've got a lot going on this week but hoping to take a closer look next week.

In addition to the README change you suggested, perhaps we should detect when /* webpackPreload: "true" */ is used and warn if so?

Do I understand correctly that adding SRI to <link rel="preload"> would not be helpful at this point?

Also, does "breaking the preloading mechanism" mean what I think it means - that the page still works, but without preloading?

I'm also curious about what you mean by double-fetch. It probably isn't necessary for me to understand but I'd like to know how that would come about in your case, if you wouldn't mind going into a bit more detail.

jscheid added a commit that referenced this issue Aug 30, 2019
Note that the test case isn't very useful as an automatic test because
behaviour differs between browser models and versions, and the test is
only run in a single Chromium version via Puppeteer. The test is still
useful when run manually, like so:

  npm install  # install requirements
  npm install webpack@4 \
    extract-text-webpack-plugin@4.0.0-alpha.0 \
    html-webpack-plugin@3  # upgrade to Webpack 4
  npm run mocha  # Run mocha tests
  (cd examples/webpack-preload/dist/ && python -m SimpleHTTPServer)
  # Visit http://localhost:8000/ with different browsers

See #111
jscheid added a commit that referenced this issue Aug 30, 2019
@jscheid
Copy link
Collaborator

jscheid commented Aug 30, 2019

I've investigated this a bit more and found the following (all tested with the current release of the plugin, 1.3.2 -- the upcoming 1.3.3 doesn't make any changes to preload handling so should be identical):

Safari 12.1.2 (14607.3.9)

No warnings, loads resource once, preloads correctly, regardless of whether integrity is specified on the link tag.

Firefox 68.0.2

No warnings, preloads correctly (requires network.preload setting to be enabled) but only if integrity is added to the link rel="preload" tag. Without integrity the resource gets loaded twice.

Chrome 76.0.3809.132

Prints a warning about integrity mismatch (and another one about the resource not being used in time) regardless of whether integrity is present on the preload tag or not. (I've tried a few things, such as HTTP/2, to see if I can make it work but doesn't seem to make a difference.)

Based on the above and on the recent spec changes discussed in the ticket you linked to, @josephliccini, we should add integrity to link tags to make things work correctly on Firefox and (hopefully) future Chrome versions.

I'm planning to release that in version 1.4.0 some time in the near future. Since the feature is either broken or behind a feature flag, it doesn't appear to be urgent. In the meantime, I've added the warning to the README as you've suggested.

jscheid added a commit that referenced this issue Aug 30, 2019
jscheid added a commit that referenced this issue Sep 27, 2019
jscheid added a commit that referenced this issue Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants