Use <link rel=“prefetch”> for prefetching #5737
Conversation
timneutkens
changed the title
timneutkens
changed the title
| // Mainly this is for Safari | ||
| // https://caniuse.com/#feat=link-rel-prefetch | ||
| // https://caniuse.com/#feat=link-rel-preload | ||
| link.rel = supportsPrefetch ? 'prefetch' : 'preload' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Created this PR: #5744
Will merge it once @yoavweiss / @igrigorik get back about this.
There was a problem hiding this comment.
Indeed preload is a better choice for same page resources. On top of that, the future of prefetch for subresources is a bit murky.
| // https://caniuse.com/#feat=link-rel-preload | ||
| link.rel = supportsPrefetch ? 'prefetch' : 'preload' | ||
| link.href = `${this.assetPrefix}/_next/static/${encodeURIComponent(this.buildId)}/pages${scriptRoute}` | ||
| link.as = 'script' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| clearCache (route) { | ||
| route = this.normalizeRoute(route) | ||
| delete this.pageCache[route] | ||
| delete this.loadingRoutes[route] | ||
| delete this.loadingRoutes[route] |
There was a problem hiding this comment.
@timneutkens this one looks duplicated, were you trying to delete from prefetchCache?
There was a problem hiding this comment.
Yeah I saw that while adding Malte's feedback, will be in my upcoming PR.
…5744) Based on #5737 (comment) This will cause a warning in chrome/safari after 3s
Fixes #5734
Note that this makes
prefetchno longer return the page object, as we're only injecting prefetch tags