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

enhance(prefetch): Adds a 'load' prefetch strategy, and ignores 3g in slow connection detection #9513

Merged
merged 13 commits into from
Jan 3, 2024

Conversation

wtto00
Copy link
Contributor

@wtto00 wtto00 commented Dec 24, 2023

Changes

  • Add global ignoreSlowConnection in prefetch config.
    Because the result of navigator.connection.effectiveType is inaccurate, or unstable, users are allowed to globally ignore this judgment.
  • defaultStrategy in prefetch config can be all.
    Allow users to preload all links on the current page by default.

Testing

I have add a e2e test in packages\astro\e2e\prefetch.test.js, named Prefetch (prefetchAll: true, defaultStrategy: 'all')

Docs

  • Users can set a global ignore slow connection detection.
  • Users can set defaultStrategy to be 'all'.

The comments in code:

{
	/**
	 * @docs
	 * @name prefetch.prefetchAll
	 * @type {boolean}
	 * @description
	 * Enable prefetching for all links, including those without the `data-astro-prefetch` attribute.
	 * This value defaults to `true` when using the `<ViewTransitions />` router. Otherwise, the default value is `false`.
	 *
	 * ```js
	 * prefetch: {
	 * 	prefetchAll: true
	 * }
	 * ```
	 *
	 * When set to `true`, you can disable prefetching individually by setting `data-astro-prefetch="false"` on any individual links.
	 *
	 * ```html
	 * <a href="/about" data-astro-prefetch="false">About</a>
	 *```
	 */
	prefetchAll?: boolean;

	/**
	 * @docs
	 * @name prefetch.defaultStrategy
	 * @type {'tap' | 'hover' | 'viewport' | 'all'}
	 * @default `'hover'`
	 * @description
	 * The default prefetch strategy to use when the `data-astro-prefetch` attribute is set on a link with no value.
	 *
	 * - `'tap'`: Prefetch just before you click on the link.
	 * - `'hover'`: Prefetch when you hover over or focus on the link. (default)
	 * - `'viewport'`: Prefetch as the links enter the viewport.
	 * - `'all'`: Prefetch the link without any restrictions.
	 *
	 * You can override this default value and select a different strategy for any individual link by setting a value on the attribute.
	 *
	 * ```html
	 * <a href="/about" data-astro-prefetch="viewport">About</a>
	 * ```
	 */
	defaultStrategy?: 'tap' | 'hover' | 'viewport' | 'all';

	/**
	 * @docs
	 * @name prefetch.ignoreSlowConnection
	 * @type {boolean}
	 * @description
	 * Ignore slow connection detection.
	 *
	 * ```js
	 * prefetch: {
	 * 	ignoreSlowConnection: true
	 * }
	 * ```
	 *
	 * When set to `true`, you can enable slow connection detection by adding `{ ignoreSlowConnection: false }` to the parameters of `prefetch` manually .
	 *
	 * ```js
	 * prefetch('/about', { ignoreSlowConnection: false });
	 *```
	 */
	ignoreSlowConnection?: boolean;
};

/cc @withastro/maintainers-docs for feedback!

Copy link

changeset-bot bot commented Dec 24, 2023

🦋 Changeset detected

Latest commit: aa3ee52

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Dec 24, 2023
@wtto00 wtto00 marked this pull request as draft December 25, 2023 00:35
@wtto00 wtto00 marked this pull request as ready for review December 25, 2023 03:52
@bluwy
Copy link
Member

bluwy commented Dec 26, 2023

  • Add global ignoreSlowConnection in prefetch config.
    Because the result of navigator.connection.effectiveType is inaccurate, or unstable, users are allowed to globally ignore this judgment.

I'm not really sure we should add this option. How would the value be inaccurate? If we detect the user is on slow connection or data saver mode, it would not be a good practice to ignore that and prefetch anyways.

  • defaultStrategy in prefetch config can be all.
    Allow users to preload all links on the current page by default.

Thanks for adding this. It's something I held off adding before since I figured there isn't a big usecase, but I don't mind if someone adds it.

Naming-wise maybe load makes more sense? So it nicely reflects how other strategies work:

  • prefetch on tap
  • prefetch on hover
  • prefetch on viewport enter
  • prefetch on page load

I'm a bit worried that all might be confusing with the prefetchAll option.

@wtto00
Copy link
Contributor Author

wtto00 commented Dec 26, 2023

@bluwy Thanks for your reply.

I have renamed all as load. I'm sorry for my poor English, load is better.

How would the value be inaccurate?

  1. The network is fluctuating. When users load a page, the network speed may be quite slow for the first one or two seconds, but then it improves. However, when the network speed improves, the page will not start preloading.
  2. My broadband is 10M, the speed is a bit slow, but it is completely sufficient to visit a page and prefetch the links. Perhaps it is more appropriate to judge whether to use limited traffic, WiFi, or broadband for this prefetch judgment.
  3. The default configuration will not have any impact, and the choice is given to developers to make the framework more flexible.

@bluwy
Copy link
Member

bluwy commented Dec 26, 2023

No need to apologize! And thanks for updating them quickly.

Re your points:

  1. The slow connection check is only ran when we execute the strategy, for example, we only check the connection when tapping or hovering an element, which can happen anytime when the connection stabilizes. Furthermore, if you're not using View Transitions, if you navigate to a new page, the script will be re-executed if it needs to re-check something.
  2. Is the navigator.connection.saveData or navigator.connection.effectiveType check blocking your broadband? We can definitely tweak it since 10M sounds reasonable to me.
  3. I'm leaning towards lesser config options so we can bake in best practices, hence I'd like to avoid a new option if possible. We can tweak the checks in no2 so it works for your case.

@wtto00
Copy link
Contributor Author

wtto00 commented Dec 26, 2023

  1. Yes, you are right. But I was using View Transitions and the config of defaultStrategy is 'viewport'. So I refresh the page, sometimes it can be preloaded, sometimes it cannot be preloaded, and most of the time it cannot be. I think it might be mainly because of my slow and unstable internet speed.
  2. I executed navigator.connection in the console, and got the following result:
    {
      downlink: 0.4,
      effectiveType: "3g",
      onchange: null,
      rtt: 400,
      saveData: false
    }
    But after a while, the result changed to:
    {
      downlink: 0.4,
      effectiveType: "4g",
      onchange: null,
      rtt: 250,
      saveData: false
    }
    "3g" and "4g" are constantly switching. Perhaps using /2g/ for matching is better than /(2|3)g/.
  3. I agress with you. Most of the time, simple and less is better.

@wtto00
Copy link
Contributor Author

wtto00 commented Dec 27, 2023

Hello, @bluwy I tested navigator.connection in another network environment. The result is:

{
  downlink: 1.3,
  effectiveType: "3g",
  onchange: null,
  rtt: 850,
  saveData: false
}

Or sometimes "4g".

But at the same time, I also tested the internet speed on https://fast.com/ and https://www.speedtest.net/, and the result was 86Mbps.

So I think the results of the navigator.connection are not accurate and unstable.

I'm not sure if the result of navigator.connection is related to the current website being accessed, or if it only represents local network information?

@bluwy
Copy link
Member

bluwy commented Dec 27, 2023

I think navigator.connection should reflect your internet connection itself. Thanks for testing the results. In that case, I'm fine with dropping the 3g check then, and we can check for 2g only. I think Chrome would sometimes estimate the "effectiveType" especially on Wifi as there's no concept of g, and the estimation isn't probably right.

@wtto00
Copy link
Contributor Author

wtto00 commented Dec 27, 2023

@bluwy Hello, I have resubmitted the code.

  • Remove the global configuration item ignoreSlowConnection.
  • Ignore 3g in slow connection detection.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I pushed a commit to clean up the changesets a bit. The rest looks great! Thanks for implementing the feature. Since this includes a feature, we'll release this together in the next minor, which should come around the next couple weeks when the rest return from the holidays.

At the meantime, feel free to also update the prefetch docs about the new load strategy at https://github.com/withastro/docs/edit/main/src/content/docs/en/guides/prefetch.mdx

@bluwy bluwy added the semver: minor Change triggers a `minor` release label Dec 27, 2023
@wtto00 wtto00 changed the title [enhance:prefetch] Add global ignoreSlowConnection and 'none' to defaultStrategy enum enhance(prefetch): Adds a 'load' prefetch strategy, and ignores 3g in slow connection detection Dec 27, 2023
wtto00 added a commit to wtto00/docs that referenced this pull request Dec 27, 2023
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

@bluwy does this PR require some changes in the docs?

@bluwy
Copy link
Member

bluwy commented Jan 2, 2024

Yeah it's updated at withastro/docs#5967

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

For visibility, docs is approving the changeset!

@Princesseuh Princesseuh added this to the 4.1.0 milestone Jan 2, 2024
@ematipico ematipico merged commit e44f6ac into withastro:main Jan 3, 2024
13 of 14 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants