Skip to content

Commit

Permalink
Fix #11107 - don't prefetch preloaded modules (#22818)
Browse files Browse the repository at this point in the history
This PR proposes a fix for #11107 (JS modules are loaded twice). A more detailed explanation of the investigation that led to this PR can be found in the issue's comments (#11107 (comment)).

## Replicability

To identify that the issue replicates on any given project, you need to 
1. look at the network tab (first/clean load of site, so preferably ⌘+⇧+R on an incognito tab), 
2. sort by "name", and filter requests by `mime-type:application/javascript` (selecting "JS" in the devtools filters will actually show all "script" types, but ignore all "javascript" types)
3. look for pairs of identical calls with one originating from initial HTML (`preload` of priority "high" originating from "(index)" or "([page name])")  and another one from a script (`prefetch` of priority "lowest" originating from a .js file), where neither of the files is served from the cache.

Here's a screenshot of an example of what to look for:
<img width="601" alt="Screen Shot 2021-03-07 at 09 59 18" src="https://user-images.githubusercontent.com/1325721/110234627-cf1c6d00-7f2b-11eb-9cd7-749bf881ba56.png">


The issue was reproduced easily on the following projects:
- On [nextjs.org](https://nextjs.org/) where duplicates add up to ~70kB of transferred javascript out of 470kB (14.9%).
- On [vercel.com](https://vercel.com/) where duplicates add up to ~105kB of transferred javascript out of 557kB (18.8%).
- On [tiktok.com](https://tiktok.com/en) where duplicates add up to ~514kB of transferred javascript out of 1556kB (33%).
- In my own project using `"next": "^10.0.1"` (private repo) where duplicates add up to about 5% of total transferred javascript.
- In the issue's comments, a developer reported a replication using `"^10.0.7"` on a [public repo](https://github.com/SidOfc/sidneyliebrand.io).


## Some information about the fix

- Both `preload` and `prefetch` values for `<link rel="x">` behave similarly, with the difference being in network priority level (preload is high priority, prefetch is lowest priority).

- Next.js uses `<link rel="preload">` in its initial HTML but then *only* uses `<link rel="prefetch">` for the rest of the lifetime of the page. 

- However, when Next.js detects that a script should be requested in advance, it only checks for matching `<link rel="prefetch">` and not `<link rel="preload">` (which have higher priority and are present earlier in the DOM, thus have a greater likelihood of being already loaded). 

This PR aims to fix that oversight.

## Potential issues (none AFAIK)

As far as I can tell by looking through the codebase, **there is no downside** not to add a `prefetch` when a `preload` is already in the DOM. No other script looks for a `<link>` based on its `rel` attribute.
  • Loading branch information
Sheraff committed Sep 19, 2021
1 parent 638e6cc commit 76dee14
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 1 deletion.
6 changes: 5 additions & 1 deletion packages/next/client/route-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,11 @@ function prefetchViaDom(
link?: HTMLLinkElement
): Promise<any> {
return new Promise<void>((res, rej) => {
if (document.querySelector(`link[rel="prefetch"][href^="${href}"]`)) {
const selector = `
link[rel="prefetch"][href^="${href}"],
link[rel="preload"][href^="${href}"],
script[src^="${href}"]`
if (document.querySelector(selector)) {
return res()
}

Expand Down
23 changes: 23 additions & 0 deletions test/integration/preload-viewport/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,29 @@ describe('Prefetching Links in viewport', () => {
expect(found).toBe(false)
})

it('should not prefetch already loaded scripts', async () => {
const browser = await webdriver(appPort, '/')

const scriptSrcs = await browser.eval(`(function() {
return Array.from(document.querySelectorAll('script'))
.map(function(el) {
return el.src && new URL(el.src).pathname
}).filter(Boolean)
})()`)

await browser.eval('next.router.prefetch("/")')

const linkHrefs = await browser.eval(`(function() {
return Array.from(document.querySelectorAll('link'))
.map(function(el) {
return el.href && new URL(el.href).pathname
}).filter(Boolean)
})()`)

console.log({ linkHrefs, scriptSrcs })
expect(linkHrefs.some((href) => scriptSrcs.includes(href))).toBe(false)
})

it('should not duplicate prefetches', async () => {
const browser = await webdriver(appPort, '/multi-prefetch')

Expand Down

0 comments on commit 76dee14

Please sign in to comment.