-
Notifications
You must be signed in to change notification settings - Fork 26.5k
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
Fix #11107 - don't prefetch preloaded modules #22818
Fix #11107 - don't prefetch preloaded modules #22818
Conversation
Fix vercel#11107 - don't prefetch preloaded modules
This comment has been minimized.
This comment has been minimized.
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? |
This comment has been minimized.
This comment has been minimized.
Up! |
@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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This comment has been minimized.
This comment has been minimized.
Failing test suitesCommit: 9da5e17 test/integration/image-component/default/test/index.test.js
Expand output● Image Component Tests › dev mode › should warn when img with layout=fill is inside a container without position relative
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 "we use script tags with the defer prop instead of preloads now"
@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. |
This comment has been minimized.
This comment has been minimized.
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | Sheraff/next.js updated-duplicate-module-load | Change | |
---|---|---|---|
buildDuration | 13.9s | 14s | |
buildDurationCached | 3.7s | 3.5s | -216ms |
nodeModulesSize | 182 MB | 182 MB |
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 |
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 | |
webpack-HASH.js gzip | 1.45 kB | 1.45 kB | ✓ |
Overall change | 70.6 kB | 70.6 kB |
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 |
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 | |
/error-in-render avg req/sec | 1762.88 | 1718.1 |
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 | |
webpack-HASH.js gzip | 1.65 kB | 1.65 kB | ✓ |
Overall change | 101 kB | 101 kB |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and added a test, thanks for investigating this!
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
mime-type:application/javascript
(selecting "JS" in the devtools filters will actually show all "script" types, but ignore all "javascript" types)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:
"next": "^10.0.1"
(private repo) where duplicates add up to about 5% of total transferred javascript."^10.0.7"
on a public repo.Some information about the fix
Both
preload
andprefetch
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 apreload
is already in the DOM. No other script looks for a<link>
based on itsrel
attribute.