Skip to content
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

Refactor i18n checks on request handling #27328

Merged
merged 5 commits into from
Jul 21, 2021

Conversation

javivelasco
Copy link
Member

@javivelasco javivelasco commented Jul 20, 2021

Currently there is a lot of mutation in the Next.js Server and the checks for Locale are directly coded in the general request handler. Ideally, we should have a function where we just pass the request input (url + headers + config) and generate a bunch of metadata that analyzes it generating all metadata we might require for both the URL and i18n + basePath information.

This PR brings:

  • A new parsing function parseUrl that joins parsing an absolute/relative URL into a data structure compatible with the Node parsing output but missing redundant properties.
  • A wrapper parseNextURL that extends parseUrl analyzing i18n and basePath based on the provided configuration, url and headers. This function is pure and stateless so it can be used outside of the Next.js context.
  • Types improvements and reuse.
  • Refactors next-server.ts request handling using the above mentioned functions so that the code there just apply effects to the req object and the parsedUrl.query leaving the code much more straightforward.
  • Refactors getRouteRegex decomposing in two different functions where getParametrizedRoute can be used to retrieve the serializable data that is used to generate the Regex.

@javivelasco javivelasco force-pushed the refactor-locale-detection branch 2 times, most recently from 48e4b04 to 34f5186 Compare July 20, 2021 15:30
@vercel vercel deleted a comment from ijjk Jul 20, 2021
@vercel vercel deleted a comment from ijjk Jul 20, 2021
@javivelasco javivelasco force-pushed the refactor-locale-detection branch 4 times, most recently from 463a465 to aedb2b7 Compare July 21, 2021 06:48
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@vercel vercel deleted a comment from ijjk Jul 21, 2021
@javivelasco javivelasco marked this pull request as ready for review July 21, 2021 10:15
@ijjk
Copy link
Member

ijjk commented Jul 21, 2021

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary javivelasco/next.js refactor-locale-detection Change
buildDuration 16.8s 16.3s -473ms
buildDurationCached 3.8s 3.6s -214ms
nodeModulesSize 49.5 MB 49.5 MB ⚠️ +8.35 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary javivelasco/next.js refactor-locale-detection Change
/ failed reqs 0 0
/ total time (seconds) 2.795 2.88 ⚠️ +0.08
/ avg req/sec 894.44 868.19 ⚠️ -26.25
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.696 1.787 ⚠️ +0.09
/error-in-render avg req/sec 1474.14 1399.34 ⚠️ -74.8
Client Bundles (main, webpack, commons) Overall increase ⚠️
vercel/next.js canary javivelasco/next.js refactor-locale-detection 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 20.9 kB 21 kB ⚠️ +82 B
webpack-HASH.js gzip 1.53 kB 1.53 kB
Overall change 67.8 kB 67.9 kB ⚠️ +82 B
Legacy Client Bundles (polyfills)
vercel/next.js canary javivelasco/next.js refactor-locale-detection 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 javivelasco/next.js refactor-locale-detection 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.61 kB 5.61 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 javivelasco/next.js refactor-locale-detection Change
_buildManifest.js gzip 490 B 490 B
Overall change 490 B 490 B
Rendered Page Sizes Overall decrease ✓
vercel/next.js canary javivelasco/next.js refactor-locale-detection Change
index.html gzip 531 B 530 B -1 B
link.html gzip 544 B 542 B -2 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB -3 B

Diffs

Diff for main-HASH.js
@@ -5890,6 +5890,7 @@
       Object.defineProperty(exports, "__esModule", {
         value: true
       });
+      exports.getParametrizedRoute = getParametrizedRoute;
       exports.getRouteRegex = getRouteRegex; // this isn't importing the escape-string-regex module
       // to reduce bytes
 
@@ -5917,10 +5918,8 @@
         };
       }
 
-      function getRouteRegex(normalizedRoute) {
-        var segments = (normalizedRoute.replace(/\/$/, "") || "/")
-          .slice(1)
-          .split("/");
+      function getParametrizedRoute(route) {
+        var segments = (route.replace(/\/$/, "") || "/").slice(1).split("/");
         var groups = {};
         var groupIndex = 1;
         var parameterizedRoute = segments
@@ -5957,11 +5956,29 @@
         }
 
         return {
-          re: new RegExp("^".concat(parameterizedRoute, "(?:/)?$")),
+          parameterizedRoute: parameterizedRoute,
           groups: groups
         };
       }
 
+      function getRouteRegex(normalizedRoute) {
+        var result = getParametrizedRoute(normalizedRoute);
+
+        if ("routeKeys" in result) {
+          return {
+            re: new RegExp("^".concat(result.parameterizedRoute, "(?:/)?$")),
+            groups: result.groups,
+            routeKeys: result.routeKeys,
+            namedRegex: "^".concat(result.namedParameterizedRoute, "(?:/)?$")
+          };
+        }
+
+        return {
+          re: new RegExp("^".concat(result.parameterizedRoute, "(?:/)?$")),
+          groups: result.groups
+        };
+      }
+
       /***/
     },
Diff for index.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-286fcf1894d7baae7f35.js"
+      src="/_next/static/chunks/main-01e3b7fe776dde9adc16.js"
       defer=""
     ></script>
     <script
Diff for link.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-286fcf1894d7baae7f35.js"
+      src="/_next/static/chunks/main-01e3b7fe776dde9adc16.js"
       defer=""
     ></script>
     <script
Diff for withRouter.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-286fcf1894d7baae7f35.js"
+      src="/_next/static/chunks/main-01e3b7fe776dde9adc16.js"
       defer=""
     ></script>
     <script

Webpack 4 Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary javivelasco/next.js refactor-locale-detection Change
buildDuration 13.6s 12.9s -707ms
buildDurationCached 5.1s 4.8s -299ms
nodeModulesSize 49.5 MB 49.5 MB ⚠️ +8.35 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary javivelasco/next.js refactor-locale-detection Change
/ failed reqs 0 0
/ total time (seconds) 2.838 2.798 -0.04
/ avg req/sec 881.05 893.36 +12.31
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.702 1.811 ⚠️ +0.11
/error-in-render avg req/sec 1468.67 1380.15 ⚠️ -88.52
Client Bundles (main, webpack, commons) Overall increase ⚠️
vercel/next.js canary javivelasco/next.js refactor-locale-detection Change
17.HASH.js gzip 2.98 kB 2.98 kB
18.HASH.js gzip 185 B 185 B
677f882d2ed8..HASH.js gzip 13.7 kB 13.7 kB ⚠️ +70 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.4 kB 68.5 kB ⚠️ +70 B
Legacy Client Bundles (polyfills)
vercel/next.js canary javivelasco/next.js refactor-locale-detection 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 javivelasco/next.js refactor-locale-detection 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 javivelasco/next.js refactor-locale-detection Change
_buildManifest.js gzip 500 B 500 B
Overall change 500 B 500 B
Rendered Page Sizes Overall decrease ✓
vercel/next.js canary javivelasco/next.js refactor-locale-detection Change
index.html gzip 577 B 576 B -1 B
link.html gzip 587 B 587 B
withRouter.html gzip 569 B 568 B -1 B
Overall change 1.73 kB 1.73 kB -2 B

Diffs

Diff for 677f882d2ed8..c4df.HASH.js
@@ -3471,6 +3471,7 @@
       Object.defineProperty(exports, "__esModule", {
         value: true
       });
+      exports.getParametrizedRoute = getParametrizedRoute;
       exports.getRouteRegex = getRouteRegex; // this isn't importing the escape-string-regex module
       // to reduce bytes
 
@@ -3498,10 +3499,8 @@
         };
       }
 
-      function getRouteRegex(normalizedRoute) {
-        var segments = (normalizedRoute.replace(/\/$/, "") || "/")
-          .slice(1)
-          .split("/");
+      function getParametrizedRoute(route) {
+        var segments = (route.replace(/\/$/, "") || "/").slice(1).split("/");
         var groups = {};
         var groupIndex = 1;
         var parameterizedRoute = segments
@@ -3538,11 +3537,29 @@
         }
 
         return {
-          re: new RegExp("^".concat(parameterizedRoute, "(?:/)?$")),
+          parameterizedRoute: parameterizedRoute,
           groups: groups
         };
       }
 
+      function getRouteRegex(normalizedRoute) {
+        var result = getParametrizedRoute(normalizedRoute);
+
+        if ("routeKeys" in result) {
+          return {
+            re: new RegExp("^".concat(result.parameterizedRoute, "(?:/)?$")),
+            groups: result.groups,
+            routeKeys: result.routeKeys,
+            namedRegex: "^".concat(result.namedParameterizedRoute, "(?:/)?$")
+          };
+        }
+
+        return {
+          re: new RegExp("^".concat(result.parameterizedRoute, "(?:/)?$")),
+          groups: result.groups
+        };
+      }
+
       /***/
     },
Diff for index.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.726ef802be8b1e2738a9.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.478e7617d8bdd88fdbea.js"
       defer=""
     ></script>
     <script
Diff for link.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.726ef802be8b1e2738a9.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.478e7617d8bdd88fdbea.js"
       defer=""
     ></script>
     <script
Diff for withRouter.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.726ef802be8b1e2738a9.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.478e7617d8bdd88fdbea.js"
       defer=""
     ></script>
     <script
Commit: 124264e

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@kodiakhq kodiakhq bot merged commit e65c56e into vercel:canary Jul 21, 2021
@javivelasco javivelasco deleted the refactor-locale-detection branch July 21, 2021 16:13
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
Currently there is a lot of mutation in the Next.js Server and the checks for Locale are directly coded in the general request handler. Ideally, we should have a function where we just pass the request input (url + headers + config) and generate a bunch of metadata that analyzes it generating all metadata we might require for both the URL and i18n + basePath information.

This PR brings:
- A new parsing function `parseUrl` that joins parsing an absolute/relative URL into a data structure compatible with the Node parsing output but missing redundant properties.
- A wrapper `parseNextURL` that extends `parseUrl` analyzing `i18n` and `basePath` based on the provided configuration, url and headers. This function is pure and stateless so it can be used outside of the Next.js context.
- Types improvements and reuse.
- Refactors `next-server.ts` request handling using the above mentioned functions so that the code there just apply effects to the `req` object and the `parsedUrl.query` leaving the code much more straightforward.
- Refactors `getRouteRegex` decomposing in two different functions where `getParametrizedRoute` can be used to retrieve the serializable data that is used to generate the Regex.
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants