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(gatsby,gatsby-link): add queue to prefetch #33530

Merged
merged 4 commits into from
Oct 18, 2021
Merged

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented Oct 14, 2021

Description

We make prefetch less eager by removing links that are out of the viewport. If you're a fast scroller we prefetch links that get quickly scrolled over. By adding a 3s delay before we prefetch this feature becomes more valuable.

This is a small part of a bigger refactor but this gets us in a good spot for a first V4 release.

Documentation

Related Issues

[ch38877]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 14, 2021
Comment on lines +166 to +173
this.io = createIntersectionObserver(ref, inViewPort => {
if (inViewPort) {
this.abortPrefetch = this._prefetch()
} else {
if (this.abortPrefetch) {
this.abortPrefetch.abort()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removes prefetch when link gets out of viewport. No need to add it to the queue

if (!this.prefetchTriggered.has(pagePath)) {
this.apiRunner(`onPrefetchPathname`, { pathname: pagePath })
this.prefetchTriggered.add(pagePath)
if (this.prefetchTriggered.has(pagePath)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if already prefetched this is a no-op but we keep the signature

prefetches.then(() => {
setTimeout(() => {
this._throttlePrefetch()
}, 3000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait 3 seconds for the next batch - we should add config options for these things.

}

_throttlePrefetch() {
const idleCallback = window.requestIdleCallback || (cb => setTimeout(cb, 0))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Start Prefetch when idle

@wardpeet wardpeet added topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 14, 2021
const idleCallback = window.requestIdleCallback || (cb => setTimeout(cb, 0))

idleCallback(() => {
const toPrefetch = this.prefetchQueued.splice(0, 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

To me this is a bit weird - we create a batch here, but then later we might skip some that were already prefetched - this looks to me like we might create a lot of batches that do NO-OP, but because of construction, we might multiple 3s intervals until prefetching actually do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True but it's difficult to do this correclty otherwise because a prefetch can get aborted and remove from the queue so checking if it's part of the queue beforehand might negate in never prefetching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I can do when it's already prefetched, remove all instances of the path in the queue

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if loader.prefetch would dedupe "pending" prefetches (and not just those that already started prefetch processing) this would be solved as then this.prefetchQueued would not get duplicates anymore, but either way, this might not be big problem here and maybe it can stay as is (and we look into it again if we see problem with this setup for prefetching)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider this example: I have a link in the header and a link at the bottom of the viewprot both going to page A. Both pages get inside the prefetch queue. If I scroll down before 3s, the first link gets aborted thus not requested. Now page A still gets prefetched because we had 2 links.

If I understand you correctly, this is what you saying, right?
Move prefetchedQueued to a set so these paths get deduped? This would mean page a will never be prefetched.

What we should in the future do is when a link is starting to prefetch remove other instances if present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants