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

fix(gatsby-plugin-offline): skip prefetching all resources #16691

Merged
merged 4 commits into from
Sep 24, 2019

Conversation

rexxars
Copy link
Contributor

@rexxars rexxars commented Aug 16, 2019

Description

If you have a plugin which uses the setHeadComponents API to add link-tags with a preconnect rel, the offline plugin will schedule it to be prefetched. This leads to the situation where instead of simply opening a connection to the specified host, it instead triggers a GET-request.

I'm not 100% clear on what should happen if you have prefetch or prerender links, but I feel like skipping them is probably a better option that to add another prefetch for them? Would love to hear your thoughts on this, though.

Related Issues

Fixes #15883

@rexxars rexxars requested a review from kkemple August 16, 2019 21:11
@rexxars rexxars requested a review from a team as a code owner August 16, 2019 21:11
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

This is a great change. This will fix over fetching assets which is great! I added a few comments which I think makes this change a little bit better. Feel free to decide otherwise 😉 I'm happy to hear your thoughts.

packages/gatsby-plugin-offline/src/gatsby-browser.js Outdated Show resolved Hide resolved
packages/gatsby-plugin-offline/src/gatsby-browser.js Outdated Show resolved Hide resolved
@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Sep 2, 2019
@LekoArts
Copy link
Contributor

Hi @rexxars 👋 Did you find time to address Ward's comments? Would be awesome to have this merged in! Thanks a lot :)

@rexxars
Copy link
Contributor Author

rexxars commented Sep 16, 2019

Will have a look later today!

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

LGTM after changes! 👍 Thanks, this is a great change! We reduce bandwidth and lower the amount of logs saying that a preload has been triggered but left unused.

@rexxars
Copy link
Contributor Author

rexxars commented Sep 23, 2019

Thanks for fixing, I kind of forgot about this 😊

@wardpeet wardpeet removed the status: awaiting author response Additional information has been requested from the author label Sep 24, 2019
@wardpeet
Copy link
Contributor

I'm going to merge this one, if we need to rollback, we'll rollback :)

@wardpeet wardpeet changed the title fix(gatsby-plugin-offline): skip prefetching preconnect resources fix(gatsby-plugin-offline): skip prefetching all resources Sep 24, 2019
@wardpeet wardpeet merged commit e688b0c into gatsbyjs:master Sep 24, 2019
@rexxars rexxars deleted the offline-dont-fetch-preconnect branch November 5, 2019 19:16
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.

[gatsby-plugin-offline] creates link prefetch from link preconnect
4 participants