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(prefetch): add support for effective-connection-type & saveData #10365

Merged
merged 4 commits into from
Dec 11, 2018

Conversation

addyosmani
Copy link
Contributor

Sending a PR for this after chatting with @KyleAMathews :)

This change introduces support for network and data-savings related checks to the Gatsby prefetcher.

  • If a browser supports navigator.connection.effectiveType, we will not prefetch if the user is on a connection that is effectively slow-2g or 2g. As users on these connections already have constrained bandwidth, limiting this feature to 3g, 4g and better seems reasonable.

  • If a browser supports the saveData client-hint (e.g. Chromium based browsers) and the user has indicated they prefer to save data (e.g. are on a constrained data plan), we also don't prefetch in those cases. Data Saving is primarily turned on in NBU regions and I don't anticipate this change having any negative effect for users with the feature off.

These changes are ones I've already made to a prefetcher we're using in an upcoming project called quicklink and think they'd benefit Gatsby users too :)

@addyosmani addyosmani requested a review from a team as a code owner December 8, 2018 21:06
@pieh
Copy link
Contributor

pieh commented Dec 9, 2018

This is awesome!

I do think we should use it earlier - particularly here

if (!apiRunner)
console.error(`Run setApiRunnerForLoader() before enqueing paths`)
// Tell plugins with custom prefetching logic that they should start
// prefetching this path.
onPrefetchPathname(path)
// If a plugin has disabled core prefetching, stop now.
if (disableCorePrefetching.some(a => a)) {
return false
}

Probably even before onPrefetchPathname(path) to not let plugins actually override this behaviour (or need to re-implement it).

Bailing in the ./prefetch would cause some weird effects with service worker integration - service worker would think we have resources for a page. @davidbailey00 please correct me if I'm wrong on this

@pieh
Copy link
Contributor

pieh commented Dec 10, 2018

@addyosmani Is there way to simulate navigator.connection.effectiveType/navigator.connection.saveData values? I briefly tried assigning different values in chrome, but that seems to be no-op (I still get 4g/false respectively). It would be great to test this without extra conditions for bailing early

@vtenfys
Copy link
Contributor

vtenfys commented Dec 10, 2018

@pieh Hmm changing the network type in the Chrome dev tools works for me

image

image

@pieh
Copy link
Contributor

pieh commented Dec 10, 2018

That'll work for manual testing - I don't think we can do the same with cypress. This kind of features are often done and once and then not touched anymore - it would be great to use some e2e tests to avoid regressions in this department in any future changes to runtime code

@addyosmani
Copy link
Contributor Author

addyosmani commented Dec 10, 2018

@pieh Thanks for taking a look! If I'm interpreting your feedback correctly, would moving these changes to

(L195) be the right call here?

Re: testing, I have a little less experience with Cypress, but we do also provide flags in Chrome for forcing effective connection values (e.g slow-2g). Do you have the ability to control those via Cypress?

image

@KyleAMathews
Copy link
Contributor

This is great @addyosmani thanks!

@pieh
Copy link
Contributor

pieh commented Dec 10, 2018

That's entry point for prefetching logic, so I think it makes sense to bail early there. I will still need to verify locally, so I will push any changes to this PR.

It seems we can pass cmdline args to Chrome with Cypress (https://docs.cypress.io/api/plugins/browser-launch-api.html#Usage) and if https://peter.sh/experiments/chromium-command-line-switches/ is correct --force-effective-connection-type is something we can use.

pieh added 3 commits December 11, 2018 17:35
this will also skip trigerring any prefetch related APIs, so offline plugin won't be confused
we are testing 2 scenarios: with normal connection type and with forced 2g connection type
2g connection should not prefetch anything
@KyleAMathews
Copy link
Contributor

Yeah very cool we can test this @pieh! It'd be fun to see if we can figure out performance tests w/ this in the future.

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Thanks @addyosmani 🙏

@pieh pieh merged commit 1c51831 into gatsbyjs:master Dec 11, 2018
@gatsbot
Copy link

gatsbot bot commented Dec 11, 2018

Holy buckets, @addyosmani — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…atsbyjs#10365)

This change introduces support for network and data-savings related checks to the Gatsby prefetcher. 

* If a browser supports [navigator.connection.effectiveType](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/effectiveType), we will not prefetch if the user is on a connection that is effectively `slow-2g` or `2g`. As users on these connections already have constrained bandwidth, limiting this feature to `3g`, `4g` and better seems reasonable.

* If a browser supports the [saveData](WICG/netinfo#42) client-hint (e.g. Chromium based browsers) and the user has indicated they prefer to save data (e.g. are on a constrained data plan), we also don't prefetch in those cases. Data Saving is primarily turned on in NBU regions and I don't anticipate this change having any negative effect for users with the feature off.
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.

4 participants