fix: Make router ready in case of custom _app getInitialProps #27473

raon0211 · 2021-07-25T07:49:54Z

From #20628, when the page is rendered server-side, Router's isReady field needs to be initially set to true. However, when _app has custom getInitialProps, it seems that it is not the case, even though the page is rendered on the server.

This leads to a bug that Router.isReady is never set to true.

This pull request fixes the problem by fixing the initial calculation logic of isReady of Router.

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

Feature

Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
Related issues linked using fixes #number
Integration tests added
Documentation added
Telemetry added. In case of a feature if it's used or not.
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes

ijjk · 2021-07-25T08:18:23Z

Failing test suites

Commit: ba6d8c4

test/integration/router-is-ready-app-gip/test/index.test.js

Expand output

● Test suite failed to run

TypeError [ERR_INVALID_ARG_TYPE]: The "data" argument must be of type string or an instance of Buffer, TypedArray, or DataView. Received null

  416 |       this.originalContent = content
  417 |     }
> 418 |     writeFileSync(this.path, content, 'utf8')
      |     ^
  419 |   }
  420 |
  421 |   replace(pattern, newValue) {

  at File.write (lib/next-test-utils.js:418:5)
  at File.restore (lib/next-test-utils.js:448:10)
  at integration/router-is-ready-app-gip/test/index.test.js:41:19

ijjk · 2021-07-25T16:21:16Z

packages/next/server/render.tsx

+  const routerIsReady = !!(
+    getServerSideProps ||
+    hasPageGetInitialProps ||
+    !defaultAppGetInitialProps


Suggested change

!defaultAppGetInitialProps

(!defaultAppGetInitialProps && !isSSG)

A page with getStaticProps isn't considered ready until we're on the client and can determine if a query is present or not even with getInitialProps in _app. See here for related query updating that makes the getStaticProps page ready.

Thanks! I applied the change in 5cc4ff8.

ijjk · 2021-07-25T16:21:52Z

test/integration/router-is-ready-app-gip/pages/appGip.js

@@ -0,0 +1,23 @@
+import { useRouter } from 'next/router'


Can we add another page that uses getStaticProps to assert the behavior describe in https://github.com/vercel/next.js/pull/27473/files#r676169393 is correct

I added some tests in 5cc4ff8. 🙇

ijjk

Changes and added tests look good, thanks!

ijjk · 2021-07-26T19:48:01Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
buildDuration	16.3s	16.4s	⚠️ +148ms
buildDurationCached	3.7s	3.7s	⚠️ +47ms
nodeModulesSize	50.3 MB	50.3 MB	⚠️ +309 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.775	2.697	-0.08
/ avg req/sec	900.96	927.1	+26.14
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.668	1.524	-0.14
/error-in-render avg req/sec	1498.35	1640.28	+141.93

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

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
359.HASH.js gzip	2.96 kB	2.96 kB	✓
745.HASH.js gzip	180 B	180 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	21 kB	21 kB	⚠️ +9 B
webpack-HASH.js gzip	1.53 kB	1.53 kB	✓
Overall change	67.9 kB	67.9 kB	⚠️ +9 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
_app-HASH.js gzip	803 B	803 B	✓
_error-HASH.js gzip	3.06 kB	3.06 kB	✓
amp-HASH.js gzip	554 B	554 B	✓
css-HASH.js gzip	329 B	329 B	✓
dynamic-HASH.js gzip	2.52 kB	2.52 kB	✓
head-HASH.js gzip	2.28 kB	2.28 kB	✓
hooks-HASH.js gzip	903 B	903 B	✓
image-HASH.js gzip	5.63 kB	5.63 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	319 B	319 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	19.1 kB	19.1 kB	✓

Client Build Manifests

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
_buildManifest.js gzip	489 B	489 B	✓
Overall change	489 B	489 B	✓

Rendered Page Sizes Overall increase ⚠️

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
index.html gzip	530 B	531 B	⚠️ +1 B
link.html gzip	544 B	544 B	✓
withRouter.html gzip	524 B	525 B	⚠️ +1 B
Overall change	1.6 kB	1.6 kB	⚠️ +2 B

Diffs

Diff for main-HASH.js

@@ -3998,6 +3998,7 @@
           this.isReady = !!(
             self.__NEXT_DATA__.gssp ||
             self.__NEXT_DATA__.gip ||
+            (self.__NEXT_DATA__.appGip && !self.__NEXT_DATA__.gsp) ||
             (!autoExportDynamic && !self.location.search && !false)
           );
           this.isPreview = !!isPreview;

Diff for index.html

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

Diff for link.html

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

Diff for withRouter.html

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

Webpack 4 Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
buildDuration	12.9s	12.9s	-29ms
buildDurationCached	4.9s	5s	⚠️ +43ms
nodeModulesSize	50.3 MB	50.3 MB	⚠️ +309 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.801	2.83	⚠️ +0.03
/ avg req/sec	892.65	883.27	⚠️ -9.38
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.546	1.518	-0.03
/error-in-render avg req/sec	1617.35	1646.47	+29.12

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

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
17.HASH.js gzip	2.98 kB	2.98 kB	✓
18.HASH.js gzip	185 B	185 B	✓
677f882d2ed8..HASH.js gzip	13.8 kB	13.8 kB	⚠️ +11 B
framework.HASH.js gzip	41.9 kB	41.9 kB	✓
main-HASH.js gzip	8.4 kB	8.4 kB	✓
webpack-HASH.js gzip	1.22 kB	1.22 kB	✓
Overall change	68.5 kB	68.5 kB	⚠️ +11 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
polyfills-HASH.js gzip	31.3 kB	31.3 kB	✓
Overall change	31.3 kB	31.3 kB	✓

Client Pages

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
_app-HASH.js gzip	791 B	791 B	✓
_error-HASH.js gzip	3.76 kB	3.76 kB	✓
amp-HASH.js gzip	552 B	552 B	✓
css-HASH.js gzip	333 B	333 B	✓
dynamic-HASH.js gzip	2.71 kB	2.71 kB	✓
head-HASH.js gzip	2.97 kB	2.97 kB	✓
hooks-HASH.js gzip	911 B	911 B	✓
index-HASH.js gzip	231 B	231 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	298 B	298 B	✓
script-HASH.js gzip	3.02 kB	3.02 kB	✓
withRouter-HASH.js gzip	294 B	294 B	✓
e025d2764813..52f.css gzip	125 B	125 B	✓
Overall change	17.6 kB	17.6 kB	✓

Client Build Manifests

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
_buildManifest.js gzip	500 B	500 B	✓
Overall change	500 B	500 B	✓

Rendered Page Sizes Overall increase ⚠️

	vercel/next.js canary	raon0211/next.js fix-router-ready	Change
index.html gzip	575 B	577 B	⚠️ +2 B
link.html gzip	589 B	588 B	-1 B
withRouter.html gzip	569 B	569 B	✓
Overall change	1.73 kB	1.73 kB	⚠️ +1 B

Diffs

Diff for 677f882d2ed8..c4df.HASH.js

@@ -1717,6 +1717,7 @@
           this.isReady = !!(
             self.__NEXT_DATA__.gssp ||
             self.__NEXT_DATA__.gip ||
+            (self.__NEXT_DATA__.appGip && !self.__NEXT_DATA__.gsp) ||
             (!autoExportDynamic && !self.location.search && !false)
           );
           this.isPreview = !!isPreview;

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.67199c98ca31edd10b07.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.fdbbc994a48a1224f695.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.67199c98ca31edd10b07.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.fdbbc994a48a1224f695.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.67199c98ca31edd10b07.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.fdbbc994a48a1224f695.js"
       defer=""
     ></script>
     <script

Commit: 4edf424

…#27473) From vercel#20628, when the page is rendered server-side, `Router`'s `isReady` field needs to be initially set to `true`. However, when `_app` has custom `getInitialProps`, it seems that it is not the case, even though the page is rendered on the server. This leads to a bug that `Router.isReady` is never set to `true`. This pull request fixes the problem by fixing the initial calculation logic of `isReady` of `Router`. ## Bug - [ ] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md` ## Feature - [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [ ] Errors have helpful link attached, see `contributing.md` ## Documentation / Examples - [x] Make sure the linting passes

Make router ready in case of global _app getInitialProps

a53ed77

raon0211 requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners July 25, 2021 07:49

ijjk added the type: next label Jul 25, 2021

raon0211 changed the title ~~fix: Make router ready in case of global _app getInitialProps~~ fix: Make router ready in case of custom _app getInitialProps Jul 25, 2021

fix typo

ba6d8c4

This comment has been minimized.

Sign in to view

fix test error

c67f6ce

This comment has been minimized.

Sign in to view

ijjk reviewed Jul 25, 2021

View reviewed changes

Set router.isReady to false when page has getStaticProps

5cc4ff8

raon0211 requested a review from ijjk July 26, 2021 01:15

This comment has been minimized.

Sign in to view

ijjk approved these changes Jul 26, 2021

View reviewed changes

Merge branch 'canary' into fix-router-ready

4edf424

kodiakhq bot merged commit 23ac435 into vercel:canary Jul 26, 2021

raon0211 deleted the fix-router-ready branch July 27, 2021 01:57

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Make router ready in case of custom _app getInitialProps #27473

fix: Make router ready in case of custom _app getInitialProps #27473

raon0211 commented Jul 25, 2021 •

edited

Loading

This comment has been minimized.

ijjk commented Jul 25, 2021

This comment has been minimized.

ijjk Jul 25, 2021

raon0211 Jul 26, 2021

ijjk Jul 25, 2021

raon0211 Jul 26, 2021

This comment has been minimized.

ijjk left a comment

ijjk commented Jul 26, 2021

Diffs

Diffs

	!defaultAppGetInitialProps
	(!defaultAppGetInitialProps && !isSSG)

fix: Make router ready in case of custom _app getInitialProps #27473

fix: Make router ready in case of custom _app getInitialProps #27473

Conversation

raon0211 commented Jul 25, 2021 • edited Loading

Bug

Feature

Documentation / Examples

This comment has been minimized.

ijjk commented Jul 25, 2021

Failing test suites

This comment has been minimized.

ijjk Jul 25, 2021

Choose a reason for hiding this comment

raon0211 Jul 26, 2021

Choose a reason for hiding this comment

ijjk Jul 25, 2021

Choose a reason for hiding this comment

raon0211 Jul 26, 2021

Choose a reason for hiding this comment

This comment has been minimized.

ijjk left a comment

Choose a reason for hiding this comment

ijjk commented Jul 26, 2021

Stats from current PR

Diffs

Diffs

raon0211 commented Jul 25, 2021 •

edited

Loading