Improve code quality of _document #12342

Kiarash-Z · 2020-04-30T09:21:03Z

Hi, this PR includes some improvements for the code quality of _document.tsx and _app.tsx as follows:

_app.tsx:

Avoid repetition of if (process.env.NODE_ENV !== 'production') warnUrl() in createUrl by abstracting the logic into warnWhenNotProduction function
Avoid repetition of getters for router's pathname, asPath, and query properties by creating them using Object.defineProperty based on a new defined variable routerData

_document.tsx:

Refine getPageFile function by unifying its return type using a new startingUrl variable
Replace some multi-level variable check with optional chaining
Remove unnecessary use of map(the returned value wasn't used) and replace it with filter and forEach in Head class
Refine getAmpPath function by removing unnecessary ternary operator and replacing it with || operator
Simplify getPreloadMainLinks's render

timneutkens · 2020-04-30T09:24:23Z

Avoid repetition of if (process.env.NODE_ENV !== 'production') warnUrl() in createUrl by abstracting the logic into warnWhenNotProduction function

These were there for a reason, it causes terser to throw the lines away in production whereas the added function is not thrown away.

Avoid repetition of getters for router's pathname, asPath, and query properties by creating them using Object.defineProperty based on a new defined variable routerData

Let's not make changes to deprecated features that could potentially break. createUrl has been deprecated for 1.5 years now.

ijjk · 2020-04-30T09:28:39Z

Stats from current PR

Default Server Mode (Increase detected ⚠️)

General Overall increase ⚠️

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
buildDuration	12.9s	13s	⚠️ +113ms
nodeModulesSize	55.2 MB	55.2 MB	⚠️ +50 B

Page Load Tests Overall increase ✓

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.297	2.263	-0.03
/ avg req/sec	1088.18	1104.53	⚠️ +16.35
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.363	1.328	-0.03
/error-in-render avg req/sec	1833.88	1882.51	⚠️ +48.63

Client Bundles (main, webpack, commons)

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
main-HASH.js gzip	6.28 kB	6.28 kB	✓
webpack-HASH.js gzip	746 B	746 B	✓
de003c3a9d30..5232.js gzip	10.3 kB	10.3 kB	✓
framework.a1..NSE.txt gzip	220 B	220 B	✓
framework.a1..NSE.txt gzip	220 B	220 B	✓
framework.HASH.js gzip	39.2 kB	39.2 kB	✓
Overall change	57 kB	57 kB	✓

Client Bundles (main, webpack, commons) Modern

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
main-HASH.module.js gzip	4.82 kB	4.82 kB	✓
webpack-HASH..dule.js gzip	746 B	746 B	✓
de003c3a9d30..dule.js gzip	6.88 kB	6.88 kB	✓
framework.HA..dule.js gzip	39.2 kB	39.2 kB	✓
Overall change	51.7 kB	51.7 kB	✓

Legacy Client Bundles (polyfills)

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
polyfills-HASH.js gzip	26.3 kB	26.3 kB	✓
Overall change	26.3 kB	26.3 kB	✓

Client Pages Overall increase ⚠️

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
_app.js gzip	1.23 kB	1.28 kB	⚠️ +48 B
_error.js gzip	3.13 kB	3.13 kB	✓
hooks.js gzip	663 B	663 B	✓
index.js gzip	222 B	222 B	✓
link.js gzip	2.06 kB	2.06 kB	✓
routerDirect.js gzip	280 B	280 B	✓
withRouter.js gzip	278 B	278 B	✓
Overall change	7.86 kB	7.91 kB	⚠️ +48 B

Client Pages Modern Overall increase ⚠️

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
_app.module.js gzip	598 B	640 B	⚠️ +42 B
_error.module.js gzip	2.09 kB	2.09 kB	✓
hooks.module.js gzip	383 B	383 B	✓
index.module.js gzip	223 B	223 B	✓
link.module.js gzip	1.52 kB	1.52 kB	✓
routerDirect..dule.js gzip	279 B	279 B	✓
withRouter.m..dule.js gzip	278 B	278 B	✓
Overall change	5.37 kB	5.41 kB	⚠️ +42 B

Client Build Manifests

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
_buildManifest.js gzip	61 B	61 B	✓
_buildManife..dule.js gzip	61 B	61 B	✓
Overall change	122 B	122 B	✓

Rendered Page Sizes

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
index.html gzip	926 B	926 B	✓
link.html gzip	937 B	937 B	✓
withRouter.html gzip	925 B	925 B	✓
Overall change	2.79 kB	2.79 kB	✓

Diffs

Diff for _app.js

@@ -166,51 +166,51 @@ function Container(p) {
 }
 
 function createUrl(router) {
-  // This is to make sure we don't references the router object at call time
-  var pathname = router.pathname,
-      asPath = router.asPath,
-      query = router.query;
-  return {
-    get query() {
-      if (false) {}
-      return query;
-    },
-
-    get pathname() {
-      if (false) {}
-      return pathname;
-    },
-
-    get asPath() {
-      if (false) {}
-      return asPath;
-    },
+  var warnWhenNotProduction = function warnWhenNotProduction() {
+    if (false) {}
+  };
 
+  var url = {
     back: function back() {
-      if (false) {}
+      warnWhenNotProduction();
       router.back();
     },
     push: function push(url, as) {
-      if (false) {}
+      warnWhenNotProduction();
       return router.push(url, as);
     },
     pushTo: function pushTo(href, as) {
-      if (false) {}
+      warnWhenNotProduction();
       var pushRoute = as ? href : '';
       var pushUrl = as || href;
       return router.push(pushRoute, pushUrl);
     },
     replace: function replace(url, as) {
-      if (false) {}
+      warnWhenNotProduction();
       return router.replace(url, as);
     },
     replaceTo: function replaceTo(href, as) {
-      if (false) {}
+      warnWhenNotProduction();
       var replaceRoute = as ? href : '';
       var replaceUrl = as || href;
       return router.replace(replaceRoute, replaceUrl);
     }
+  }; // This is to make sure we don't references the router object at call time
+
+  var routerData = {
+    pathname: router.pathname,
+    asPath: router.asPath,
+    query: router.query
   };
+  Object.keys(routerData).forEach(function (key) {
+    Object.defineProperty(url, key, {
+      get: function get() {
+        warnWhenNotProduction();
+        return routerData[key];
+      }
+    });
+  });
+  return url;
 }
 
 /***/ }),

Diff for _app.module.js

@@ -99,53 +99,51 @@ function Container(p) {
 }
 
 function createUrl(router) {
-  // This is to make sure we don't references the router object at call time
-  var {
-    pathname,
-    asPath,
-    query
-  } = router;
-  return {
-    get query() {
-      if (false) {}
-      return query;
-    },
-
-    get pathname() {
-      if (false) {}
-      return pathname;
-    },
-
-    get asPath() {
-      if (false) {}
-      return asPath;
-    },
+  var warnWhenNotProduction = () => {
+    if (false) {}
+  };
 
+  var url = {
     back: () => {
-      if (false) {}
+      warnWhenNotProduction();
       router.back();
     },
     push: (url, as) => {
-      if (false) {}
+      warnWhenNotProduction();
       return router.push(url, as);
     },
     pushTo: (href, as) => {
-      if (false) {}
+      warnWhenNotProduction();
       var pushRoute = as ? href : '';
       var pushUrl = as || href;
       return router.push(pushRoute, pushUrl);
     },
     replace: (url, as) => {
-      if (false) {}
+      warnWhenNotProduction();
       return router.replace(url, as);
     },
     replaceTo: (href, as) => {
-      if (false) {}
+      warnWhenNotProduction();
       var replaceRoute = as ? href : '';
       var replaceUrl = as || href;
       return router.replace(replaceRoute, replaceUrl);
     }
+  }; // This is to make sure we don't references the router object at call time
+
+  var routerData = {
+    pathname: router.pathname,
+    asPath: router.asPath,
+    query: router.query
   };
+  Object.keys(routerData).forEach(key => {
+    Object.defineProperty(url, key, {
+      get: () => {
+        warnWhenNotProduction();
+        return routerData[key];
+      }
+    });
+  });
+  return url;
 }
 
 /***/ })

Serverless Mode (Increase detected ⚠️)

General Overall increase ⚠️

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
buildDuration	14s	13.9s	-136ms
nodeModulesSize	55.2 MB	55.2 MB	⚠️ +50 B

Client Bundles (main, webpack, commons)

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
main-HASH.js gzip	6.28 kB	6.28 kB	✓
webpack-HASH.js gzip	746 B	746 B	✓
de003c3a9d30..5232.js gzip	10.3 kB	10.3 kB	✓
framework.a1..NSE.txt gzip	220 B	220 B	✓
framework.a1..NSE.txt gzip	220 B	220 B	✓
framework.HASH.js gzip	39.2 kB	39.2 kB	✓
Overall change	57 kB	57 kB	✓

Client Bundles (main, webpack, commons) Modern

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
main-HASH.module.js gzip	4.82 kB	4.82 kB	✓
webpack-HASH..dule.js gzip	746 B	746 B	✓
de003c3a9d30..dule.js gzip	6.88 kB	6.88 kB	✓
framework.HA..dule.js gzip	39.2 kB	39.2 kB	✓
Overall change	51.7 kB	51.7 kB	✓

Legacy Client Bundles (polyfills)

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
polyfills-HASH.js gzip	26.3 kB	26.3 kB	✓
Overall change	26.3 kB	26.3 kB	✓

Client Pages Overall increase ⚠️

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
_app.js gzip	1.23 kB	1.28 kB	⚠️ +48 B
_error.js gzip	3.13 kB	3.13 kB	✓
hooks.js gzip	663 B	663 B	✓
index.js gzip	222 B	222 B	✓
link.js gzip	2.06 kB	2.06 kB	✓
routerDirect.js gzip	280 B	280 B	✓
withRouter.js gzip	278 B	278 B	✓
Overall change	7.86 kB	7.91 kB	⚠️ +48 B

Client Pages Modern Overall increase ⚠️

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
_app.module.js gzip	598 B	640 B	⚠️ +42 B
_error.module.js gzip	2.09 kB	2.09 kB	✓
hooks.module.js gzip	383 B	383 B	✓
index.module.js gzip	223 B	223 B	✓
link.module.js gzip	1.52 kB	1.52 kB	✓
routerDirect..dule.js gzip	279 B	279 B	✓
withRouter.m..dule.js gzip	278 B	278 B	✓
Overall change	5.37 kB	5.41 kB	⚠️ +42 B

Client Build Manifests

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
_buildManifest.js gzip	61 B	61 B	✓
_buildManife..dule.js gzip	61 B	61 B	✓
Overall change	122 B	122 B	✓

Serverless bundles Overall increase ⚠️

	zeit/next.js canary	Kiarash-Z/next.js improve-code-quality	Change
_error.js	557 kB	558 kB	⚠️ +359 B
404.html	4.18 kB	4.18 kB	✓
hooks.html	3.81 kB	3.81 kB	✓
index.js	557 kB	558 kB	⚠️ +359 B
link.js	595 kB	595 kB	⚠️ +359 B
routerDirect.js	587 kB	588 kB	⚠️ +359 B
withRouter.js	588 kB	588 kB	⚠️ +359 B
Overall change	2.89 MB	2.89 MB	⚠️ +1.79 kB

ijjk · 2020-04-30T09:31:25Z

Failing test suites

test/integration/client-navigation/test/index.test.js

Client Navigation > With url property > Should keep immutable pathname, asPath and query

Expand output

● Client Navigation › With url property › Should keep immutable pathname, asPath and query

expect(received).toMatchObject(expected)

- Expected
+ Received

- Object {
-   "asPath": "/nav/url-prop-change?added=yes",
-   "pathname": "/nav/url-prop-change",
-   "query": Object {
-     "added": "yes",
-   },
- }
+ Object {}

  131 |         .text()
  132 | 
> 133 |       expect(JSON.parse(urlResult)).toMatchObject({
      |                                     ^
  134 |         query: { added: 'yes' },
  135 |         pathname: '/nav/url-prop-change',
  136 |         asPath: '/nav/url-prop-change?added=yes',

  at Object.<anonymous> (integration/client-navigation/test/index.test.js:133:37)

Kiarash-Z · 2020-04-30T09:35:54Z

@timneutkens Thanks for the review. I reverted the changes for _app.tsx 😊

Kiarash-Z · 2020-05-01T09:19:08Z

@timneutkens I guess there's an issue with the 2 failing checks. The "Generate Pull Request Stats..." one fails because of error Couldn't find any versions for "@next/react-refresh-utils" that matches "9.3.7-canary.1 which I think has nothing to do with this PR 🤔 and the "Build, test, and deploy / lint (pull_request)" check fails because of a linting error in _app.tsx which I reverted the changes for.(Only modified file of this PR is _document.tsx)

packages/next/pages/_document.tsx

Co-authored-by: Maxim <maxim@flexify.tech>

Timer

Thanks!

Kiarash-Z added 3 commits April 30, 2020 13:09

Improve _document's code quality

fffd5c3

Improve _app's code quality

c65d1fe

Simplify getPreloadMainLinks render

55359d5

Kiarash-Z requested review from ijjk, lfades, Timer and timneutkens as code owners April 30, 2020 09:21

Revert the abstraction changes for _app.tsx

4bc29e5

chore: add the empty line at the end of _app.tsx

8d00dcc

unbelivr reviewed May 2, 2020

View reviewed changes

packages/next/pages/_document.tsx Outdated Show resolved Hide resolved

Timer and others added 2 commits May 1, 2020 23:58

Update packages/next/pages/_document.tsx

b525457

Co-authored-by: Maxim <maxim@flexify.tech>

Merge branch 'canary' into improve-code-quality

ba934a6

Timer changed the title ~~Improve code quality of _document and _app~~ Improve code quality of _document May 2, 2020

Timer added this to the 9.3.7 milestone May 2, 2020

Timer added the type: next label May 2, 2020

Timer approved these changes May 2, 2020

View reviewed changes

Timer merged commit 943209c into vercel:canary May 2, 2020

Timer modified the milestones: 9.3.7, 9.4 May 11, 2020

rokinsky pushed a commit to rokinsky/next.js that referenced this pull request Jul 11, 2020

Improve code quality of _document (vercel#12342)

2679ecc

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve code quality of _document #12342

Improve code quality of _document #12342

Kiarash-Z commented Apr 30, 2020 •

edited

timneutkens commented Apr 30, 2020

ijjk commented Apr 30, 2020

Diffs

ijjk commented Apr 30, 2020

Kiarash-Z commented Apr 30, 2020

Kiarash-Z commented May 1, 2020

Timer left a comment

Improve code quality of _document #12342

Improve code quality of _document #12342

Conversation

Kiarash-Z commented Apr 30, 2020 • edited

timneutkens commented Apr 30, 2020

ijjk commented Apr 30, 2020

Stats from current PR

Diffs

ijjk commented Apr 30, 2020

Failing test suites

Kiarash-Z commented Apr 30, 2020

Kiarash-Z commented May 1, 2020

Timer left a comment

Choose a reason for hiding this comment

Kiarash-Z commented Apr 30, 2020 •

edited