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

Add link=preload support #344

Closed
wants to merge 17 commits into from

Conversation

TheRoadyBuddha
Copy link

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

This pull request adds <link rel="preload"> support, per this issue: #142.

This is to support recent changes in Chrome Lighthouse that strongly hurt performance scores due to not using rel="preload" with asynchronous chunks, as well as improve page load performance. (More details provided https://developers.google.com/web/tools/lighthouse/audits/preload).

Fallbacks are provided for all browsers that do not support rel="preload".

Breaking Changes

No breaking changes.

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Jan 24, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


jgillespie seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@alexander-akait
Copy link
Member

alexander-akait commented Jan 24, 2019

@TheRoadyBuddha Thanks for PR,

that strongly hurt performance scores due to not using rel="preload" with asynchronous chunks

i think we should do preload for async chunk always (without options), can you provide link on documentation

Other solution use webpack comments for async chunk and get information about preload, as we do in webpack for js https://webpack.js.org/guides/code-splitting/#prefetching-preloading-modules

Also please accept CLA and add tests

@TheRoadyBuddha
Copy link
Author

@evilebottnawi Documentation has been changed, CLA signed, and now preload is the default behavior for any browser that supports it. On the tests, all the existing tests pass (the failing CI test above has to do with an npm vulnerability not related to this PR). I'm struggling to find a test for this that doesn't replicate a test that is already in this test suite, since the behavior should mirror master today. Was there something specific you had in mind?

@alexander-akait
Copy link
Member

You need add new test for new feature

@alexander-akait
Copy link
Member

Also please read #344 (comment)

@wietsedevries
Copy link

@TheRoadyBuddha This would be such a great feature and you're almost there! Don't give up 🥇

@simonjoom
Copy link

simonjoom commented Aug 20, 2019

Thanks working for me in my repo, and pass the lighthouse error :) it should to be merged i think ?no? for info i use gatsby i just changed the core of minicss-extract-plugin to match this patch.

@TheRoadyBuddha
Copy link
Author

TheRoadyBuddha commented Aug 20, 2019 via email

@starksider starksider mentioned this pull request Dec 30, 2019
6 tasks
@TotallWAR
Copy link

Any updates?

@jglesner
Copy link

+1 would really like to have this feature

@TheRoadyBuddha
Copy link
Author

@evilebottnawi I've added the appropriate test updates. rel="preload" is the default behavior (it will still fall back to the original behavior in older browsers. I've also singed the CLA and all linting passes locally. Let me know if you have any questions. Better late than never on this one!

@TheRoadyBuddha
Copy link
Author

Hi there, is there anything additional that needs to be done to get this merged?

@rzemk
Copy link

rzemk commented Sep 28, 2020

I've been quietly following this, is there anything needed to get this merged and released?

@tamebadger
Copy link

what @rzemk said ^ would be really nice to get this in or #488

ScriptedAlchemy pushed a commit to faceyspacey/extract-css-chunks-webpack-plugin that referenced this pull request Oct 15, 2020
* WIP - preload CSS

Inspired by webpack-contrib/mini-css-extract-plugin#344

* feat: preload css

* test(TestCases): re-enable logging
@mprochkaruk
Copy link

For those who are still waiting, there is workaround for mini-css-extract-plugin > 1.2.0 :

      new MiniCssExtractPlugin({
        ...yourConfig,
        insert: (linkTag) => {
            const preloadLinkTag = document.createElement('link')
            preloadLinkTag.rel = 'preload'
            preloadLinkTag.as = 'style'
            preloadLinkTag.href = linkTag.href
            document.head.appendChild(preloadLinkTag)
            document.head.appendChild(linkTag)
        }
      })

`execLinkTag.href = ${mainTemplate.requireFn}.p + ${linkHrefPath};`,
'execLinkTag.rel = "stylesheet";',
'execLinkTag.type = "text/css";',
'document.body.appendChild(execLinkTag);',

Choose a reason for hiding this comment

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

Hi!

It's look like breaking change, because at line#344 is checking an existing link, and prevent append new link at this case.

But now, even if the link exists on the page, we get an additional unnecessary link.

Maybe better to add extra flag, checking that line#356 already append preload link?

Choose a reason for hiding this comment

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

Also, we had a problem in out internal framework with this changes, because we append preload and stylesheet links directly in document.head, with some custom onload logic, and after package updating, i see a broken integration test with new link tag inside body)

@PodaruDragos
Copy link

hello guys !!
@TheRoadyBuddha @alexander-akait is there any way we can rebase and land this ?
does things still need to be done here ?

@alexander-akait
Copy link
Member

Fixed in master, release soon

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 this pull request may close these issues.