Fix #11107 - don't prefetch preloaded modules #22818

Sheraff · 2021-03-06T10:10:09Z

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

look at the network tab (first/clean load of site, so preferably ⌘+⇧+R on an incognito tab),
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)
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:

The issue was reproduced easily on the following projects:

On nextjs.org where duplicates add up to ~70kB of transferred javascript out of 470kB (14.9%).
On vercel.com where duplicates add up to ~105kB of transferred javascript out of 557kB (18.8%).
On tiktok.com 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.

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.

Fix vercel#11107 - don't prefetch preloaded modules

SidOfc · 2021-03-06T12:23:10Z

Hi @Sheraff!

Thank you for looking into this so quickly, I just woke up to this PR, is there any way for me to try it out?

Sheraff · 2021-05-09T05:47:59Z

Up!
I believe this 1-line PR to be a very cheap way to greatly reduce the amount of data going through the wire and is worth taking a look at. Please 🥺

Sheraff · 2021-08-20T08:01:30Z

@Timer Hello sorry for pinging you like this but you're the only reviewer who commented on the original issue. Could you tell me if this could ever be merged? It seems like an extremely small fix if you look at the lines changed! And based on my analysis above, the issue is still reproduced.

And if not, maybe just a word on why not?

Thanks!

styfle

Thanks for the PR!

We'll run the tests to make sure this doesn't break anything 👍

Also, please add a test fixture to ensure this is working properly, for example css-client-nav

ijjk · 2021-08-24T20:47:29Z

Failing test suites

Commit: 9da5e17

test/integration/image-component/default/test/index.test.js

Image Component Tests > dev mode > should warn when img with layout=fill is inside a container without position relative

Expand output

● Image Component Tests › dev mode › should warn when img with layout=fill is inside a container without position relative

expect(received).toMatch(expected)

Expected pattern: /Image with src (.*)jpg(.*) may not render properly with a parent using position:\\"static\\". Consider changing the parent style to position:\\"relative\\"/gm
Received string:  ""

  605 |         .join('\n')
  606 |       expect(await hasRedbox(browser)).toBe(false)
> 607 |       expect(warnings).toMatch(
      |                        ^
  608 |         /Image with src (.*)jpg(.*) may not render properly with a parent using position:\\"static\\". Consider changing the parent style to position:\\"relative\\"/gm
  609 |       )
  610 |     })

  at Object.<anonymous> (integration/image-component/default/test/index.test.js:607:24)

packages/next/client/route-loader.ts

ijjk

Can we add a test that watches for requests when loading a page with a link to the page itself in the viewport and ensure the client bundle for that page and any other bundles are only loaded once?

Example test where the next-server is wrapped and the requests can be watched can be seen here

@ijjk

@ijjk "we use script tags with the defer prop instead of preloads now"

Sheraff · 2021-08-31T08:32:47Z

@ijjk I'm having a lot of trouble installing the project on my machine so it might be a while until I find the time to write the tests. Just mentioning it so you can prioritize this depending on how important this PR is to you.

…-module-load

ijjk · 2021-09-19T17:25:11Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
buildDuration	13.9s	14s	⚠️ +120ms
buildDurationCached	3.7s	3.5s	-216ms
nodeModulesSize	182 MB	182 MB	⚠️ +200 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.977	2.961	-0.02
/ avg req/sec	839.82	844.21	+4.39
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.452	1.454	0
/error-in-render avg req/sec	1721.57	1719.05	⚠️ -2.52

Client Bundles (main, webpack, commons) Overall increase ⚠️

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
779.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	26.7 kB	26.8 kB	⚠️ +36 B
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	70.6 kB	70.6 kB	⚠️ +36 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
_app-HASH.js gzip	977 B	977 B	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	311 B	311 B	✓
css-HASH.js gzip	328 B	328 B	✓
dynamic-HASH.js gzip	2.67 kB	2.67 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	918 B	918 B	✓
image-HASH.js gzip	4.14 kB	4.14 kB	✓
index-HASH.js gzip	260 B	260 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	386 B	386 B	✓
withRouter-HASH.js gzip	319 B	319 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	13 kB	13 kB	✓

Client Build Manifests

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
_buildManifest.js gzip	493 B	493 B	✓
Overall change	493 B	493 B	✓

Rendered Page Sizes Overall decrease ✓

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
index.html gzip	540 B	538 B	-2 B
link.html gzip	552 B	550 B	-2 B
withRouter.html gzip	533 B	532 B	-1 B
Overall change	1.63 kB	1.62 kB	-5 B

Diffs

Diff for main-HASH.js

@@ -2638,11 +2638,12 @@
 
       function prefetchViaDom(href, as, link) {
         return new Promise(function(res, rej) {
-          if (
-            document.querySelector(
-              'link[rel="prefetch"][href^="'.concat(href, '"]')
-            )
-          ) {
+          var selector = '\n      link[rel="prefetch"][href^="'
+            .concat(href, '"],\n      link[rel="preload"][href^="')
+            .concat(href, '"],\n      script[src^="')
+            .concat(href, '"]');
+
+          if (document.querySelector(selector)) {
             return res();
           }

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-4223bec09a728ba73975.js"
+      src="/_next/static/chunks/main-b119ca76bd478b712255.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-4223bec09a728ba73975.js"
+      src="/_next/static/chunks/main-b119ca76bd478b712255.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-4223bec09a728ba73975.js"
+      src="/_next/static/chunks/main-b119ca76bd478b712255.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
buildDuration	7.2s	6.9s	-325ms
buildDurationCached	3.5s	3.4s	-82ms
nodeModulesSize	182 MB	182 MB	⚠️ +200 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.952	2.899	-0.05
/ avg req/sec	846.91	862.38	+15.47
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.418	1.455	⚠️ +0.04
/error-in-render avg req/sec	1762.88	1718.1	⚠️ -44.78

Client Bundles (main, webpack, commons) Overall increase ⚠️

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
675-HASH.js gzip	13.8 kB	13.8 kB	✓
770.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	50.7 kB	50.7 kB	✓
main-HASH.js gzip	34.6 kB	34.6 kB	⚠️ +27 B
webpack-HASH.js gzip	1.65 kB	1.65 kB	✓
Overall change	101 kB	101 kB	⚠️ +27 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
_app-HASH.js gzip	1.32 kB	1.32 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	315 B	315 B	✓
css-HASH.js gzip	331 B	331 B	✓
dynamic-HASH.js gzip	2.8 kB	2.8 kB	✓
head-HASH.js gzip	356 B	356 B	✓
hooks-HASH.js gzip	637 B	637 B	✓
image-HASH.js gzip	573 B	573 B	✓
index-HASH.js gzip	262 B	262 B	✓
link-HASH.js gzip	2.2 kB	2.2 kB	✓
routerDirect..HASH.js gzip	326 B	326 B	✓
script-HASH.js gzip	393 B	393 B	✓
withRouter-HASH.js gzip	322 B	322 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	10.1 kB	10.1 kB	✓

Client Build Manifests

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
_buildManifest.js gzip	511 B	511 B	✓
Overall change	511 B	511 B	✓

Rendered Page Sizes Overall decrease ✓

	vercel/next.js canary	Sheraff/next.js updated-duplicate-module-load	Change
index.html gzip	539 B	538 B	-1 B
link.html gzip	552 B	552 B	✓
withRouter.html gzip	531 B	531 B	✓
Overall change	1.62 kB	1.62 kB	-1 B

Diffs

Diff for main-HASH.js

@@ -1938,11 +1938,11 @@
       var canPrefetch = hasPrefetch();
       function prefetchViaDom(href, as, link) {
         return new Promise(function(res, rej) {
-          if (
-            document.querySelector(
-              'link[rel="prefetch"][href^="'.concat(href, '"]')
-            )
-          ) {
+          var selector = '\n      link[rel="prefetch"][href^="'
+            .concat(href, '"],\n      link[rel="preload"][href^="')
+            .concat(href, '"],\n      script[src^="')
+            .concat(href, '"]');
+          if (document.querySelector(selector)) {
             return res();
           }
           link = document.createElement("link");

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-858a6e49cf06cd02256f.js"
+      src="/_next/static/chunks/main-67c2749aa505d94085cb.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-858a6e49cf06cd02256f.js"
+      src="/_next/static/chunks/main-67c2749aa505d94085cb.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-858a6e49cf06cd02256f.js"
+      src="/_next/static/chunks/main-67c2749aa505d94085cb.js"
       defer=""
     ></script>
     <script

Commit: 16e3f63

ijjk

I went ahead and added a test, thanks for investigating this!

Sheraff added 2 commits March 6, 2021 10:21

Fix vercel#11107 - don't prefetch preloaded modules

a1e9024

Merge pull request #1 from Sheraff/11107-duplicate-module-requests

2f3b761

Fix vercel#11107 - don't prefetch preloaded modules

Sheraff requested review from divmain, ijjk, lfades, Timer and timneutkens as code owners March 6, 2021 10:10

This comment has been minimized.

Sign in to view

Merge branch 'canary' into canary

3bdee67

ijjk added the type: next label Mar 8, 2021

This comment has been minimized.

Sign in to view

Merge branch 'canary' into canary

4ff8748

Sheraff requested a review from shuding as a code owner May 9, 2021 05:48

Merge branch 'canary' into updated-duplicate-module-load

9da5e17

Sheraff requested review from huozhi, padmaia and styfle as code owners August 20, 2021 08:05

styfle reviewed Aug 24, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

ijjk reviewed Aug 24, 2021

View reviewed changes

packages/next/client/route-loader.ts Outdated Show resolved Hide resolved

ijjk requested changes Aug 24, 2021

View reviewed changes

Sheraff added 2 commits August 30, 2021 21:44

don't preload script loaded from <script> tag

08c05a2

@ijjk "we use script tags with the defer prop instead of preloads now"

Merge branch 'canary' into updated-duplicate-module-load

057d09c

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'upstream/canary' into updated-duplicate…

f017580

…-module-load

Add regression test

16e3f63

ijjk approved these changes Sep 19, 2021

View reviewed changes

kodiakhq bot merged commit 76dee14 into vercel:canary Sep 19, 2021

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #11107 - don't prefetch preloaded modules #22818

Fix #11107 - don't prefetch preloaded modules #22818

Sheraff commented Mar 6, 2021 •

edited

This comment has been minimized.

SidOfc commented Mar 6, 2021

This comment has been minimized.

Sheraff commented May 9, 2021

Sheraff commented Aug 20, 2021

styfle left a comment

This comment has been minimized.

ijjk commented Aug 24, 2021

ijjk left a comment

Sheraff commented Aug 31, 2021

This comment has been minimized.

ijjk commented Sep 19, 2021

Diffs

Diffs

ijjk left a comment

Fix #11107 - don't prefetch preloaded modules #22818

Fix #11107 - don't prefetch preloaded modules #22818

Conversation

Sheraff commented Mar 6, 2021 • edited

Replicability

Some information about the fix

Potential issues (none AFAIK)

This comment has been minimized.

SidOfc commented Mar 6, 2021

This comment has been minimized.

Sheraff commented May 9, 2021

Sheraff commented Aug 20, 2021

styfle left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Aug 24, 2021

Failing test suites

ijjk left a comment

Choose a reason for hiding this comment

Sheraff commented Aug 31, 2021

This comment has been minimized.

ijjk commented Sep 19, 2021

Stats from current PR

Diffs

Diffs

ijjk left a comment

Choose a reason for hiding this comment

Sheraff commented Mar 6, 2021 •

edited