Fix: HMR in multi-zone handling 🌱 #59471

agustints · 2023-12-11T02:07:51Z

What?

When running a multi-zone app in dev, guest app pages would infinitely reload if you change the basePath of the host app to the default one (omit basePath settings in next.config.js) (empty string "" as per Next.js docs).

Why?

The HMR upgrade request would fail and get caught into a retry loop. In the multi-zone case, they fail because the upgrade request would be sent again for a request that had already been upgraded. This resulted in a "server.handleUpgrade() was called more than once with the same socket" error, causing the upgrade request to fail.

Every time a retry occurred, the page would trigger a full refresh since certain HMR errors cause the browser to reload.

How?

This ensures the upgrade handler only responds to requests that match the configured basePath (considering when there is no basePath). Default basePath for Next.js applications it's an empty string "".

Ref: https://nextjs.org/docs/app/api-reference/next-config-js/basePath

Other fixes & updates related to the bug:

Updated test apps to avoid having issues regarding client & server mismatch for dates
Added default use case in e2e tests, where you have a default Next.js application where the basePath it's the default one and a guest app that it's being routed by the main one through Next.js rewrites.

Closes NEXT-1797
Fixes #59161
Fixes #56615
Fixes #54454

packages/next/src/server/lib/router-server.ts

exneval · 2023-12-18T03:04:09Z

Kindly review this PR cc @ztanner 🙏

agustints · 2023-12-29T12:40:10Z

Hi @ztanner, @huozhi, could you help us here with a review?
This PR will allow many projects to be updated to the latest version, since this issue is blocking people from updating since Next.js v13.4.3.

Thank you!

ddeisadze · 2024-01-06T03:39:46Z

Any updates on this, its blocking deployments for me.

ijjk · 2024-01-06T16:46:56Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
buildDuration	12.9s	13s	N/A
buildDurationCached	7.1s	6.3s	N/A
nodeModulesSize	199 MB	199 MB	⚠️ +256 B
nextStartRea..uration (ms)	425ms	430ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
193.HASH.js gzip	181 B	182 B	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	N/A
433-HASH.js gzip	28.6 kB	28.6 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	239 B	242 B	N/A
main-HASH.js gzip	31.7 kB	31.8 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	N/A
Overall change	45.2 kB	45.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
_app-HASH.js gzip	194 B	195 B	N/A
_error-HASH.js gzip	183 B	181 B	N/A
amp-HASH.js gzip	504 B	502 B	N/A
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	255 B	253 B	N/A
head-HASH.js gzip	350 B	349 B	N/A
hooks-HASH.js gzip	369 B	369 B	✓
image-HASH.js gzip	4.28 kB	4.28 kB	N/A
index-HASH.js gzip	255 B	256 B	N/A
link-HASH.js gzip	2.61 kB	2.61 kB	✓
routerDirect..HASH.js gzip	312 B	311 B	N/A
script-HASH.js gzip	385 B	383 B	N/A
withRouter-HASH.js gzip	307 B	308 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	3.4 kB	3.4 kB	✓

Client Build Manifests

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
_buildManifest.js gzip	483 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
index.html gzip	527 B	527 B	✓
link.html gzip	541 B	539 B	N/A
withRouter.html gzip	523 B	522 B	N/A
Overall change	527 B	527 B	✓

Edge SSR bundle Size

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
edge-ssr.js gzip	93.8 kB	93.8 kB	N/A
page.js gzip	148 kB	148 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
middleware-b..fest.js gzip	621 B	624 B	N/A
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	37.4 kB	37.4 kB	N/A
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	2.07 kB	2.07 kB	✓

Next Runtimes

	vercel/next.js canary	agustints/next.js fix-hmr-dev-environment	Change
app-page-exp...dev.js gzip	169 kB	169 kB	✓
app-page-exp..prod.js gzip	95.1 kB	95.1 kB	✓
app-page-tur..prod.js gzip	95.8 kB	95.8 kB	✓
app-page-tur..prod.js gzip	90.4 kB	90.4 kB	✓
app-page.run...dev.js gzip	142 kB	142 kB	✓
app-page.run..prod.js gzip	89.7 kB	89.7 kB	✓
app-route-ex...dev.js gzip	24.1 kB	24.1 kB	✓
app-route-ex..prod.js gzip	16.7 kB	16.7 kB	✓
app-route-tu..prod.js gzip	16.7 kB	16.7 kB	✓
app-route-tu..prod.js gzip	16.3 kB	16.3 kB	✓
app-route.ru...dev.js gzip	23.5 kB	23.5 kB	✓
app-route.ru..prod.js gzip	16.3 kB	16.3 kB	✓
pages-api-tu..prod.js gzip	9.38 kB	9.38 kB	✓
pages-api.ru...dev.js gzip	9.65 kB	9.65 kB	✓
pages-api.ru..prod.js gzip	9.37 kB	9.37 kB	✓
pages-turbo...prod.js gzip	21.9 kB	21.9 kB	✓
pages.runtim...dev.js gzip	22.5 kB	22.5 kB	✓
pages.runtim..prod.js gzip	21.9 kB	21.9 kB	✓
server.runti..prod.js gzip	49.5 kB	49.5 kB	✓
Overall change	939 kB	939 kB	✓

Commit: 7cf487f

ijjk · 2024-01-06T16:48:06Z

Failing test suites

Commit: 7cf487f

pnpm test-dev test/e2e/app-dir/navigation/navigation.test.ts

app dir - navigation > query string > useParams identity between renders > should be stable in pages

Expand output

● app dir - navigation › query string › useParams identity between renders › should be stable in pages

TIMED OUT: success

fail

undefined

  626 |
  627 |   if (hardError) {
> 628 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
      |           ^
  629 |   }
  630 |   return false
  631 | }

  at check (lib/next-test-utils.ts:628:11)
  at runTests (e2e/app-dir/navigation/navigation.test.ts:88:11)
  at Object.<anonymous> (e2e/app-dir/navigation/navigation.test.ts:113:11)

Read more about building and testing Next.js in contributing.md.

ztanner · 2024-01-06T17:06:47Z

Hi @agustints, thanks for the PR! Looks like there are some failing tests, do you have time to look into them?

agustints · 2024-01-06T22:44:46Z

Hi @ztanner thank you so much for taking a look here, I appreciate that 🤝
I'm glad to let you know that I've addressed all the issues and fixed the tests that failed after your merge commit 👌

I hope we can merge this PR since it will allow many projects to be updated from Next.js v13.4.3 to the latest version 🚀
Let me know if I can help you in any way, I will be happy to help and contribute 👍

ztanner

Thanks @agustints! It still looks like one of the same tests from before is still failing. I've also left some comments around confusion I had with the logic that was introduced.

packages/next/src/server/lib/router-server.ts

agustints · 2024-01-07T19:43:24Z

Hi @ztanner thanks for your comments, I appreciate that, I updated router-server.ts logic and now seems that tests are working again 🚀

packages/next/src/server/lib/router-server.ts

…andler

…guest

…for multi-zones

ztanner

Thank you!

agustints requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners December 11, 2023 02:07

ijjk added area: tests type: next labels Dec 11, 2023

agustints force-pushed the fix-hmr-dev-environment branch from 776392e to 97d368f Compare December 11, 2023 02:32

jamaybyrone reviewed Dec 13, 2023

View reviewed changes

packages/next/src/server/lib/router-server.ts Outdated Show resolved Hide resolved

agustints requested a review from jamaybyrone December 14, 2023 03:25

agustints mentioned this pull request Dec 14, 2023

Multi-zone not working on version 13.4.19. 13.4.3 works fine #54454

Closed

1 task

9vfQbg7z4ajrGQxR mentioned this pull request Jan 4, 2024

Bug: Apps under rewrites() rules are reloaded indefinitely at the development side only. #59161

Closed

1 task

ztanner added the CI approved Approve running CI for fork label Jan 6, 2024

agustints force-pushed the fix-hmr-dev-environment branch from 80b8edd to e81e60f Compare January 6, 2024 21:46

agustints requested review from a team, padmaia and styfle as code owners January 6, 2024 21:46

agustints requested review from ismaelrumzan and removed request for jamaybyrone and a team January 6, 2024 21:46

ijjk added create-next-app Related to our CLI tool for quickly starting a new Next.js application. area: documentation examples Issue/PR related to examples Font (next/font) Related to Next.js Font Optimization. Turbopack Related to Turbopack with Next.js. labels Jan 6, 2024

agustints force-pushed the fix-hmr-dev-environment branch 2 times, most recently from c82a921 to dd94a54 Compare January 6, 2024 22:00

ztanner reviewed Jan 7, 2024

View reviewed changes

packages/next/src/server/lib/router-server.ts Outdated Show resolved Hide resolved

packages/next/src/server/lib/router-server.ts Outdated Show resolved Hide resolved

agustints force-pushed the fix-hmr-dev-environment branch from d15e3e3 to af95dea Compare January 7, 2024 19:08

agustints requested a review from ztanner January 7, 2024 19:39

ztanner reviewed Jan 8, 2024

View reviewed changes

packages/next/src/server/lib/router-server.ts Outdated Show resolved Hide resolved

agustints added 3 commits January 8, 2024 16:23

fix: Execute HMR reload only when basePath it's the same as request h…

b1cfa08

…andler

refactor: Transition app names from first and second app to host and …

03baece

…guest

test: Update existing e2e tests and considering the default use case …

c8685c4

…for multi-zones

agustints force-pushed the fix-hmr-dev-environment branch from af95dea to c8685c4 Compare January 8, 2024 19:24

agustints requested a review from ztanner January 8, 2024 20:29

ztanner approved these changes Jan 8, 2024

View reviewed changes

Merge branch 'canary' into fix-hmr-dev-environment

7cf487f

ztanner removed examples Issue/PR related to examples area: documentation create-next-app Related to our CLI tool for quickly starting a new Next.js application. Font (next/font) Related to Next.js Font Optimization. Turbopack Related to Turbopack with Next.js. labels Jan 8, 2024

ztanner enabled auto-merge (squash) January 8, 2024 23:38

ztanner merged commit 447b416 into vercel:canary Jan 8, 2024
69 checks passed

vordgi mentioned this pull request Jan 21, 2024

Remove obsolete ("remove after next minor") #60930

Open

github-actions bot added the locked label Jan 23, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: HMR in multi-zone handling 🌱 #59471

Fix: HMR in multi-zone handling 🌱 #59471

agustints commented Dec 11, 2023 •

edited

Loading

exneval commented Dec 18, 2023

agustints commented Dec 29, 2023 •

edited

Loading

ddeisadze commented Jan 6, 2024

ijjk commented Jan 6, 2024 •

edited

Loading

ijjk commented Jan 6, 2024 •

edited

Loading

ztanner commented Jan 6, 2024

agustints commented Jan 6, 2024 •

edited

Loading

ztanner left a comment

agustints commented Jan 7, 2024 •

edited

Loading

ztanner left a comment

Fix: HMR in multi-zone handling 🌱 #59471

Fix: HMR in multi-zone handling 🌱 #59471

Conversation

agustints commented Dec 11, 2023 • edited Loading

What?

Why?

How?

exneval commented Dec 18, 2023

agustints commented Dec 29, 2023 • edited Loading

ddeisadze commented Jan 6, 2024

ijjk commented Jan 6, 2024 • edited Loading

Stats from current PR

ijjk commented Jan 6, 2024 • edited Loading

Failing test suites

ztanner commented Jan 6, 2024

agustints commented Jan 6, 2024 • edited Loading

ztanner left a comment

Choose a reason for hiding this comment

agustints commented Jan 7, 2024 • edited Loading

ztanner left a comment

Choose a reason for hiding this comment

agustints commented Dec 11, 2023 •

edited

Loading

agustints commented Dec 29, 2023 •

edited

Loading

ijjk commented Jan 6, 2024 •

edited

Loading

ijjk commented Jan 6, 2024 •

edited

Loading

agustints commented Jan 6, 2024 •

edited

Loading

agustints commented Jan 7, 2024 •

edited

Loading