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.

Sign up for GitHub

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

Jump to bottom

feat(gatsby,gatsby-link): add queue to prefetch #33530

Merged
merged 4 commits into from
Oct 18, 2021
Merged

feat(gatsby,gatsby-link): add queue to prefetch #33530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e414056
feat(gatsby,gatsby-link): add queue to prefetch
wardpeet Oct 12, 2021
e05eaa2
fix tests
wardpeet Oct 14, 2021
86d15df
review fixes
wardpeet Oct 15, 2021
4474400
use correct package
wardpeet Oct 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/gatsby-legacy-polyfills/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"dist/"
],
"devDependencies": {
"yet-another-abortcontroller-polyfill": "0.0.4",
"chokidar-cli": "^3.0.0",
"codegen.macro": "^4.1.0",
"core-js": "3.9.0",
Expand Down
1 change: 1 addition & 0 deletions packages/gatsby-legacy-polyfills/src/polyfills.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ codegen`
module.exports = imports.map(file => 'import "core-js/' + file + '"').join("\\n")
`

import "yet-another-abortcontroller-polyfill"
import "whatwg-fetch"
import "url-polyfill"
import assign from "object-assign"
Expand Down
44 changes: 22 additions & 22 deletions packages/gatsby-link/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,14 @@ const createIntersectionObserver = (el, cb) => {
if (el === entry.target) {
// Check if element is within viewport, remove listener, destroy observer, and run link callback.
// MSEdge doesn't currently support isIntersecting, so also test for an intersectionRatio > 0
if (entry.isIntersecting || entry.intersectionRatio > 0) {
io.unobserve(el)
io.disconnect()
cb()
}
cb(entry.isIntersecting || entry.intersectionRatio > 0)
}
})
})

// Add element to the observer
io.observe(el)

return { instance: io, el }
}

Expand All @@ -113,6 +111,7 @@ class GatsbyLink extends React.Component {
this.state = {
IOSupported,
}
this.abortPrefetch = null
this.handleRef = this.handleRef.bind(this)
}

Expand All @@ -132,22 +131,10 @@ class GatsbyLink extends React.Component {
// Prefetch is used to speed up next navigations. When you use it on the current navigation,
// there could be a race-condition where Chrome uses the stale data instead of waiting for the network to complete
if (currentPath !== newPathName) {
___loader.enqueue(newPathName)
}
}

componentDidUpdate(prevProps, prevState) {
// Preserve non IO functionality if no support
if (this.props.to !== prevProps.to && !this.state.IOSupported) {
this._prefetch()
return ___loader.enqueue(newPathName)
}
}

componentDidMount() {
// Preserve non IO functionality if no support
if (!this.state.IOSupported) {
this._prefetch()
}
return undefined
}

componentWillUnmount() {
Expand All @@ -156,21 +143,34 @@ class GatsbyLink extends React.Component {
}
const { instance, el } = this.io

if (this.abortPrefetch) {
this.abortPrefetch.abort()
}

instance.unobserve(el)
instance.disconnect()
}

handleRef(ref) {
if (this.props.innerRef && this.props.innerRef.hasOwnProperty(`current`)) {
if (
this.props.innerRef &&
Object.prototype.hasOwnProperty.call(this.props.innerRef, `current`)
) {
this.props.innerRef.current = ref
} else if (this.props.innerRef) {
this.props.innerRef(ref)
}

if (this.state.IOSupported && ref) {
// If IO supported and element reference found, setup Observer functionality
this.io = createIntersectionObserver(ref, () => {
this._prefetch()
this.io = createIntersectionObserver(ref, inViewPort => {
if (inViewPort) {
this.abortPrefetch = this._prefetch()
} else {
if (this.abortPrefetch) {
this.abortPrefetch.abort()
}
}
Comment on lines +166 to +173
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

})
}
}
Expand Down
49 changes: 16 additions & 33 deletions packages/gatsby/cache-dir/__tests__/dev-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,26 +482,32 @@ describe(`Dev loader`, () => {
describe(`prefetch`, () => {
const flushPromises = () => new Promise(resolve => setImmediate(resolve))

it(`shouldn't prefetch when shouldPrefetch is false`, () => {
const devLoader = new DevLoader(asyncRequires, [])
it(`shouldn't prefetch when shouldPrefetch is false`, async () => {
jest.useFakeTimers()
const devLoader = new DevLoader(null, [])
devLoader.shouldPrefetch = jest.fn(() => false)
devLoader.doPrefetch = jest.fn()
devLoader.apiRunner = jest.fn()
const prefetchPromise = devLoader.prefetch(`/mypath/`)
jest.runAllTimers()

expect(devLoader.prefetch(`/mypath/`)).toBe(false)
expect(await prefetchPromise).toBe(false)
expect(devLoader.shouldPrefetch).toHaveBeenCalledWith(`/mypath/`)
expect(devLoader.apiRunner).not.toHaveBeenCalled()
expect(devLoader.doPrefetch).not.toHaveBeenCalled()
})

it(`should trigger custom prefetch logic when core is disabled`, () => {
const devLoader = new DevLoader(asyncRequires, [])
it(`should trigger custom prefetch logic when core is disabled`, async () => {
jest.useFakeTimers()
const devLoader = new DevLoader(null, [])
devLoader.shouldPrefetch = jest.fn(() => true)
devLoader.doPrefetch = jest.fn()
devLoader.apiRunner = jest.fn()
devLoader.prefetchDisabled = true

expect(devLoader.prefetch(`/mypath/`)).toBe(false)
const prefetchPromise = devLoader.prefetch(`/mypath/`)
jest.runAllTimers()
expect(await prefetchPromise).toBe(false)
expect(devLoader.shouldPrefetch).toHaveBeenCalledWith(`/mypath/`)
expect(devLoader.apiRunner).toHaveBeenCalledWith(`onPrefetchPathname`, {
pathname: `/mypath/`,
Expand All @@ -511,12 +517,14 @@ describe(`Dev loader`, () => {

it(`should prefetch when not yet triggered`, async () => {
jest.useFakeTimers()
const devLoader = new DevLoader(asyncRequires, [])
const devLoader = new DevLoader(null, [])
devLoader.shouldPrefetch = jest.fn(() => true)
devLoader.apiRunner = jest.fn()
devLoader.doPrefetch = jest.fn(() => Promise.resolve({}))
const prefetchPromise = devLoader.prefetch(`/mypath/`)
jest.runAllTimers()

expect(devLoader.prefetch(`/mypath/`)).toBe(true)
expect(await prefetchPromise).toBe(true)

// wait for doPrefetchPromise
await flushPromises()
Expand All @@ -532,30 +540,5 @@ describe(`Dev loader`, () => {
}
)
})

it(`should only run apis once`, async () => {
const devLoader = new DevLoader(asyncRequires, [])
devLoader.shouldPrefetch = jest.fn(() => true)
devLoader.apiRunner = jest.fn()
devLoader.doPrefetch = jest.fn(() => Promise.resolve({}))

expect(devLoader.prefetch(`/mypath/`)).toBe(true)
expect(devLoader.prefetch(`/mypath/`)).toBe(true)

// wait for doPrefetchPromise
await flushPromises()

expect(devLoader.apiRunner).toHaveBeenCalledTimes(2)
expect(devLoader.apiRunner).toHaveBeenNthCalledWith(
1,
`onPrefetchPathname`,
expect.anything()
)
expect(devLoader.apiRunner).toHaveBeenNthCalledWith(
2,
`onPostPrefetchPathname`,
expect.anything()
)
})
})
})
26 changes: 19 additions & 7 deletions packages/gatsby/cache-dir/__tests__/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,26 +516,32 @@ describe(`Production loader`, () => {
describe(`prefetch`, () => {
const flushPromises = () => new Promise(resolve => setImmediate(resolve))

it(`shouldn't prefetch when shouldPrefetch is false`, () => {
it(`shouldn't prefetch when shouldPrefetch is false`, async () => {
jest.useFakeTimers()
const prodLoader = new ProdLoader(null, [])
prodLoader.shouldPrefetch = jest.fn(() => false)
prodLoader.doPrefetch = jest.fn()
prodLoader.apiRunner = jest.fn()
const prefetchPromise = prodLoader.prefetch(`/mypath/`)
jest.runAllTimers()

expect(prodLoader.prefetch(`/mypath/`)).toBe(false)
expect(await prefetchPromise).toBe(false)
expect(prodLoader.shouldPrefetch).toHaveBeenCalledWith(`/mypath/`)
expect(prodLoader.apiRunner).not.toHaveBeenCalled()
expect(prodLoader.doPrefetch).not.toHaveBeenCalled()
})

it(`should trigger custom prefetch logic when core is disabled`, () => {
it(`should trigger custom prefetch logic when core is disabled`, async () => {
jest.useFakeTimers()
const prodLoader = new ProdLoader(null, [])
prodLoader.shouldPrefetch = jest.fn(() => true)
prodLoader.doPrefetch = jest.fn()
prodLoader.apiRunner = jest.fn()
prodLoader.prefetchDisabled = true

expect(prodLoader.prefetch(`/mypath/`)).toBe(false)
const prefetchPromise = prodLoader.prefetch(`/mypath/`)
jest.runAllTimers()
expect(await prefetchPromise).toBe(false)
expect(prodLoader.shouldPrefetch).toHaveBeenCalledWith(`/mypath/`)
expect(prodLoader.apiRunner).toHaveBeenCalledWith(`onPrefetchPathname`, {
pathname: `/mypath/`,
Expand All @@ -549,8 +555,10 @@ describe(`Production loader`, () => {
prodLoader.shouldPrefetch = jest.fn(() => true)
prodLoader.apiRunner = jest.fn()
prodLoader.doPrefetch = jest.fn(() => Promise.resolve({}))
const prefetchPromise = prodLoader.prefetch(`/mypath/`)
jest.runAllTimers()

expect(prodLoader.prefetch(`/mypath/`)).toBe(true)
expect(await prefetchPromise).toBe(true)

// wait for doPrefetchPromise
await flushPromises()
Expand All @@ -568,13 +576,17 @@ describe(`Production loader`, () => {
})

it(`should only run apis once`, async () => {
jest.useFakeTimers()
const prodLoader = new ProdLoader(null, [])
prodLoader.shouldPrefetch = jest.fn(() => true)
prodLoader.apiRunner = jest.fn()
prodLoader.doPrefetch = jest.fn(() => Promise.resolve({}))
const prefetchPromise = prodLoader.prefetch(`/mypath/`)
const prefetchPromise2 = prodLoader.prefetch(`/mypath/`)
jest.runAllTimers()

expect(prodLoader.prefetch(`/mypath/`)).toBe(true)
expect(prodLoader.prefetch(`/mypath/`)).toBe(true)
expect(await prefetchPromise).toBe(true)
expect(await prefetchPromise2).toBe(true)

// wait for doPrefetchPromise
await flushPromises()
Expand Down
98 changes: 79 additions & 19 deletions packages/gatsby/cache-dir/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const createPageDataUrl = rawPath => {
}

function doFetch(url, method = `GET`) {
return new Promise((resolve, reject) => {
return new Promise(resolve => {
const req = new XMLHttpRequest()
req.open(method, url, true)
req.onreadystatechange = () => {
Expand Down Expand Up @@ -98,6 +98,8 @@ export class BaseLoader {
this.inFlightDb = new Map()
this.staticQueryDb = {}
this.pageDataDb = new Map()
this.isPrefetchQueueRunning = false
this.prefetchQueued = []
this.prefetchTriggered = new Set()
this.prefetchCompleted = new Set()
this.loadComponent = loadComponent
Expand Down Expand Up @@ -396,32 +398,90 @@ export class BaseLoader {

prefetch(pagePath) {
if (!this.shouldPrefetch(pagePath)) {
return false
return {
then: resolve => resolve(false),
abort: () => {},
}
}
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

return {
then: resolve => resolve(true),
abort: () => {},
}
}

const defer = {
resolve: null,
reject: null,
promise: null,
}
defer.promise = new Promise((resolve, reject) => {
defer.resolve = resolve
defer.reject = reject
})
this.prefetchQueued.push([pagePath, defer])
const abortC = new AbortController()
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
abortC.signal.addEventListener(`abort`, () => {
const index = this.prefetchQueued.findIndex(([p]) => p === pagePath)
// remove from the queue
if (index !== -1) {
this.prefetchQueued.splice(index, 1)
}
})
pieh marked this conversation as resolved.
Show resolved Hide resolved

// Tell plugins with custom prefetching logic that they should start
// prefetching this path.
if (!this.prefetchTriggered.has(pagePath)) {
this.apiRunner(`onPrefetchPathname`, { pathname: pagePath })
this.prefetchTriggered.add(pagePath)
if (!this.isPrefetchQueueRunning) {
this.isPrefetchQueueRunning = true
setTimeout(() => {
this._processNextPrefetchBatch()
}, 3000)
}

// If a plugin has disabled core prefetching, stop now.
if (this.prefetchDisabled) {
return false
return {
then: (resolve, reject) => defer.promise.then(resolve, reject),
abort: abortC.abort.bind(abortC),
}
}

_processNextPrefetchBatch() {
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


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.

const prefetches = Promise.all(
toPrefetch.map(([pagePath, dPromise]) => {
// Tell plugins with custom prefetching logic that they should start
// prefetching this path.
if (!this.prefetchTriggered.has(pagePath)) {
this.apiRunner(`onPrefetchPathname`, { pathname: pagePath })
this.prefetchTriggered.add(pagePath)
}

// If a plugin has disabled core prefetching, stop now.
if (this.prefetchDisabled) {
return dPromise.resolve(false)
}
pieh marked this conversation as resolved.
Show resolved Hide resolved

return this.doPrefetch(findPath(pagePath)).then(() => {
if (!this.prefetchCompleted.has(pagePath)) {
this.apiRunner(`onPostPrefetchPathname`, { pathname: pagePath })
this.prefetchCompleted.add(pagePath)
}

dPromise.resolve(true)
})
})
)

const realPath = findPath(pagePath)
// Todo make doPrefetch logic cacheable
// eslint-disable-next-line consistent-return
this.doPrefetch(realPath).then(() => {
if (!this.prefetchCompleted.has(pagePath)) {
this.apiRunner(`onPostPrefetchPathname`, { pathname: pagePath })
this.prefetchCompleted.add(pagePath)
if (this.prefetchQueued.length) {
prefetches.then(() => {
setTimeout(() => {
this._processNextPrefetchBatch()
}, 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.

})
} else {
this.isPrefetchQueueRunning = false
}
})

return true
}

doPrefetch(pagePath) {
Expand Down
Loading