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

Use <link rel=“prefetch”> for prefetching #5737

Merged
merged 5 commits into from
Nov 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
83 changes: 0 additions & 83 deletions packages/next-server/lib/p-queue.js

This file was deleted.

4 changes: 1 addition & 3 deletions packages/next-server/lib/router/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import { parse, format } from 'url'
import EventEmitter from '../EventEmitter'
import shallowEquals from '../shallow-equals'
import PQueue from '../p-queue'
import { loadGetInitialProps, getURL } from '../utils'
import { _rewriteUrlForNextExport } from './'

Expand All @@ -30,7 +29,6 @@ export default class Router {
this.events = Router.events

this.pageLoader = pageLoader
this.prefetchQueue = new PQueue({ concurrency: 2 })
this.ErrorComponent = ErrorComponent
this.pathname = pathname
this.query = query
Expand Down Expand Up @@ -359,7 +357,7 @@ export default class Router {

const { pathname } = parse(url)
const route = toRoute(pathname)
return this.prefetchQueue.add(() => this.fetchRoute(route))
return this.pageLoader.prefetch(route)
}

async fetchComponent (route, as) {
Expand Down
37 changes: 35 additions & 2 deletions packages/next/lib/page-loader.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,19 @@
/* global window, document */
/* global document */
import EventEmitter from 'next-server/dist/lib/EventEmitter'

// smaller version of https://gist.github.com/igrigorik/a02f2359f3bc50ca7a9c
function listSupports (list, token) {
if (!list || !list.supports) {
return false
}
try {
return list.supports(token)
} catch (e) {
return false
}
}

const supportsPrefetch = listSupports(document.createElement('link').relList, 'prefetch')
const webpackModule = module

export default class PageLoader {
Expand All @@ -9,7 +22,7 @@ export default class PageLoader {
this.assetPrefix = assetPrefix

this.pageCache = {}
this.pageLoadedHandlers = {}
this.prefetchCache = new Set()
this.pageRegisterEvents = new EventEmitter()
this.loadingRoutes = {}
}
Expand Down Expand Up @@ -110,10 +123,30 @@ export default class PageLoader {
}
}

async prefetch (route) {
route = this.normalizeRoute(route)
const scriptRoute = route === '/' ? '/index.js' : `${route}.js`
if (this.prefetchCache.has(scriptRoute)) {
return
}
this.prefetchCache.add(scriptRoute)

const link = document.createElement('link')
// Feature detection is used to see if prefetch is supported, else fall back to preload
// Mainly this is for Safari
// https://caniuse.com/#feat=link-rel-prefetch
// https://caniuse.com/#feat=link-rel-preload
link.rel = supportsPrefetch ? 'prefetch' : 'preload'
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't semantically the same. Prefetch should only be used for future navigations (as in full page loads). For sub-resources fetches (like this one), preload is the right one to use.

Prefetch may appear to work for client side navigations, but may lead to double-downloads if the prefetch hasn't finished by the time the actual request is made. That is because preload hits the memory cache (which de-dupes requests) while prefetch is only de-duped once the response made it into the disk cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm interesting, so you're saying we should use preload instead, but that triggers a warning if not used within 3 seconds, which is likely to cause confusion to users of Next.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there really needs to be a "I know what I'm doing" flag to remove that warning. Very similar things happen with lazy loading resources before rendering where render-time depends on scroll.

CC @yoavweiss @igrigorik for actually authoritative advice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created this PR: #5744

Will merge it once @yoavweiss / @igrigorik get back about this.

Choose a reason for hiding this comment

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

Indeed preload is a better choice for same page resources. On top of that, the future of prefetch for subresources is a bit murky.

link.href = `${this.assetPrefix}/_next/static/${encodeURIComponent(this.buildId)}/pages${scriptRoute}`
link.as = 'script'
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding the importance attribute to lower priority.
https://www.chromestatus.com/feature/5273474901737472

Unfortunately when this is fixed to use preload it will use the default priority for script.

Choose a reason for hiding this comment

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

I don't think we should be using importance before it actually ships, as that can lock us out of that API shape if we'd want to change the API in some way. You're right that in terms of priority here, it'd use the default priority which is not ideal, but adding an importance attribute won't change it until it ships. I suggest we'll add it once it does.

document.head.appendChild(link)
}

clearCache (route) {
route = this.normalizeRoute(route)
delete this.pageCache[route]
delete this.loadingRoutes[route]
delete this.loadingRoutes[route]
Copy link
Contributor

@joecohens joecohens Nov 25, 2018

Choose a reason for hiding this comment

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

@timneutkens this one looks duplicated, were you trying to delete from prefetchCache?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I saw that while adding Malte's feedback, will be in my upcoming PR.


const script = document.getElementById(`__NEXT_PAGE__${route}`)
if (script) {
Expand Down
2 changes: 1 addition & 1 deletion packages/next/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"autodll-webpack-plugin": "0.4.2",
"babel-core": "7.0.0-bridge.0",
"babel-loader": "8.0.2",
"babel-plugin-react-require": "3.0.1",
"babel-plugin-react-require": "3.0.0",
"babel-plugin-transform-react-remove-prop-types": "0.4.15",
"case-sensitive-paths-webpack-plugin": "2.1.2",
"cross-spawn": "5.1.0",
Expand Down
28 changes: 28 additions & 0 deletions test/integration/production/pages/prefetch.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import Link from 'next/link'

export default () => {
return <div>
<ul>
<li>
<Link href='/' prefetch>
<a>index</a>
</Link>
</li>
<li>
<Link href='/process-env' prefetch>
<a>process env</a>
</Link>
</li>
<li>
<Link href='/counter' prefetch>
<a>counter</a>
</Link>
</li>
<li>
<Link href='/about' prefetch>
<a>about</a>
</Link>
</li>
</ul>
</div>
}
24 changes: 23 additions & 1 deletion test/integration/production/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,21 @@ describe('Production Usage', () => {
browser.close()
})

it('should add prefetch tags when Link prefetch prop is used', async () => {
const browser = await webdriver(appPort, '/prefetch')
const elements = await browser.elementsByCss('link[rel=prefetch]')
expect(elements.length).toBe(4)
await Promise.all(
elements.map(async (element) => {
const rel = await element.getAttribute('rel')
const as = await element.getAttribute('as')
expect(rel).toBe('prefetch')
expect(as).toBe('script')
})
)
browser.close()
})

it('should reload the page on page script error with prefetch', async () => {
const browser = await webdriver(appPort, '/counter')
const counter = await browser
Expand All @@ -220,7 +235,14 @@ describe('Production Usage', () => {
// Let the browser to prefetch the page and error it on the console.
await waitFor(3000)
const browserLogs = await browser.log('browser')
expect(browserLogs[0].message).toMatch(/\/no-such-page.js - Failed to load resource/)
let foundLog = false
browserLogs.forEach((log) => {
if (log.message.match(/\/no-such-page\.js - Failed to load resource/)) {
foundLog = true
}
})

expect(foundLog).toBe(true)

// When we go to the 404 page, it'll do a hard reload.
// So, it's possible for the front proxy to load a page from another zone.
Expand Down
18 changes: 10 additions & 8 deletions test/unit/router.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class PageLoader {
constructor (options = {}) {
this.options = options
this.loaded = {}
this.prefetched = {}
}

loadPage (route) {
Expand All @@ -14,6 +15,10 @@ class PageLoader {
return new Promise((resolve) => setTimeout(resolve, this.options.delay))
}
}

prefetch (route) {
this.prefetched[route] = true
}
}

describe('Router', () => {
Expand All @@ -26,10 +31,10 @@ describe('Router', () => {
const route = '/routex'
await router.prefetch(route)

expect(pageLoader.loaded['/routex']).toBeTruthy()
expect(pageLoader.prefetched['/routex']).toBeTruthy()
})

it('should only run two jobs at a time', async () => {
it('should call prefetch correctly', async () => {
global.__NEXT_DATA__ = {}
// delay loading pages for an hour
const pageLoader = new PageLoader({ delay: 1000 * 3600 })
Expand All @@ -40,11 +45,8 @@ describe('Router', () => {
router.prefetch('route3')
router.prefetch('route4')

// Wait for a bit
await new Promise((resolve) => setTimeout(resolve, 50))

expect(Object.keys(pageLoader.loaded).length).toBe(2)
expect(Object.keys(pageLoader.loaded)).toEqual(['route1', 'route2'])
expect(Object.keys(pageLoader.prefetched).length).toBe(4)
expect(Object.keys(pageLoader.prefetched)).toEqual(['route1', 'route2', 'route3', 'route4'])
})

it('should run all the jobs', async () => {
Expand All @@ -60,7 +62,7 @@ describe('Router', () => {
await router.prefetch(routes[2])
await router.prefetch(routes[3])

expect(Object.keys(pageLoader.loaded)).toEqual(routes)
expect(Object.keys(pageLoader.prefetched)).toEqual(routes)
})
})
})