Correct /_next/data link for GS(S)P with basePath #14376
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Stats from current PRDefault Server Mode (Decrease detected ✓)General Overall increase
|
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| buildDuration | 11.8s | 11.8s | |
| nodeModulesSize | 67 MB | 67 MB |
Page Load Tests Overall decrease ⚠️
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| / failed reqs | 0 | 0 | ✓ |
| / total time (seconds) | 1.959 | 1.957 | 0 |
| / avg req/sec | 1276.19 | 1277.69 | |
| /error-in-render failed reqs | 0 | 0 | ✓ |
| /error-in-render total time (seconds) | 1.167 | 1.298 | |
| /error-in-render avg req/sec | 2141.69 | 1926.27 | -215.42 |
Client Bundles (main, webpack, commons) Overall increase ⚠️
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| main-HASH.js gzip | 6.51 kB | 6.51 kB | ✓ |
| webpack-HASH.js gzip | 746 B | 746 B | ✓ |
| de003c3a9d30..4dd5.js gzip | 10.5 kB | 10.5 kB | |
| framework.HASH.js gzip | 39.1 kB | 39.1 kB | ✓ |
| Overall change | 56.9 kB | 56.9 kB |
Client Bundles (main, webpack, commons) Modern Overall decrease ✓
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| main-HASH.module.js gzip | 5.6 kB | 5.6 kB | ✓ |
| webpack-HASH..dule.js gzip | 746 B | 746 B | ✓ |
| de003c3a9d30..dule.js gzip | 6.92 kB | 6.92 kB | -2 B |
| framework.HA..dule.js gzip | 39.1 kB | 39.1 kB | ✓ |
| Overall change | 52.4 kB | 52.4 kB | -2 B |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 26.3 kB | 26.3 kB | ✓ |
| Overall change | 26.3 kB | 26.3 kB | ✓ |
Client Pages
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| routerDirect.js gzip | 279 B | 279 B | ✓ |
| withRouter.js gzip | 278 B | 278 B | ✓ |
| _error.js gzip | 3.37 kB | 3.37 kB | ✓ |
| index.js gzip | 222 B | 222 B | ✓ |
| link.js gzip | 2.05 kB | 2.05 kB | ✓ |
| hooks.js gzip | 881 B | 881 B | ✓ |
| _app.js gzip | 1.26 kB | 1.26 kB | ✓ |
| Overall change | 8.34 kB | 8.34 kB | ✓ |
Client Pages Modern
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| index.module.js gzip | 223 B | 223 B | ✓ |
| routerDirect..dule.js gzip | 281 B | 281 B | ✓ |
| withRouter.m..dule.js gzip | 278 B | 278 B | ✓ |
| hooks.module.js gzip | 383 B | 383 B | ✓ |
| _error.module.js gzip | 2.21 kB | 2.21 kB | ✓ |
| link.module.js gzip | 1.52 kB | 1.52 kB | ✓ |
| _app.module.js gzip | 604 B | 604 B | ✓ |
| Overall change | 5.49 kB | 5.49 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 270 B | 270 B | ✓ |
| _buildManife..dule.js gzip | 274 B | 274 B | ✓ |
| Overall change | 544 B | 544 B | ✓ |
Rendered Page Sizes Overall increase ⚠️
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| index.html gzip | 953 B | 955 B | |
| link.html gzip | 960 B | 961 B | |
| withRouter.html gzip | 947 B | 948 B | |
| Overall change | 2.86 kB | 2.86 kB |
Diffs
Diff for de003c3a9d30..ce095b7e0.js
@@ -750,9 +750,10 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
return path.replace(/\/$/, "") || "/";
}
- var prepareRoute = function prepareRoute(path) {
+ function prepareRoute(path) {
+ path = delBasePath(path || "");
return toRoute(!path || path === "/" ? "/index" : path);
- };
+ }
function prepareUrlAs(url, as) {
// If url and as provided as an object representation,
@@ -778,7 +779,7 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
// @ts-ignore __NEXT_DATA__
"/_next/data/"
.concat(__NEXT_DATA__.buildId)
- .concat(delBasePath(pathname), ".json")
+ .concat(pathname, ".json")
),
query: query
}),
@@ -1151,15 +1152,16 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
var _options$shallow = options.shallow,
shallow =
_options$shallow === void 0 ? false : _options$shallow;
+ var cleanedAs = delBasePath(as);
if ((0, _isDynamic.isDynamicRoute)(route)) {
- var _ref4 = (0, _url.parse)(as),
+ var _ref4 = (0, _url.parse)(cleanedAs),
asPathname = _ref4.pathname;
var routeRegex = (0, _routeRegex.getRouteRegex)(route);
var routeMatch = (0, _routeMatcher.getRouteMatcher)(
routeRegex
- )(delBasePath(asPathname || ""));
+ )(asPathname);
if (!routeMatch) {
var missingParams = Object.keys(routeRegex.groups).filter(
@@ -1210,7 +1212,7 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
}
_this2
- .set(route, pathname, query, delBasePath(as), routeInfo)
+ .set(route, pathname, query, cleanedAs, routeInfo)
.then(function() {
if (error) {
Router.events.emit("routeChangeError", error, as);Diff for de003c3a9d30..84.module.js
@@ -622,8 +622,10 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
return path.replace(/\/$/, "") || "/";
}
- var prepareRoute = path =>
- toRoute(!path || path === "/" ? "/index" : path);
+ function prepareRoute(path) {
+ path = delBasePath(path || "");
+ return toRoute(!path || path === "/" ? "/index" : path);
+ }
function prepareUrlAs(url, as) {
// If url and as provided as an object representation,
@@ -649,7 +651,7 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
// @ts-ignore __NEXT_DATA__
"/_next/data/"
.concat(__NEXT_DATA__.buildId)
- .concat(delBasePath(pathname), ".json")
+ .concat(pathname, ".json")
),
query
}),
@@ -988,12 +990,13 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
var route = toRoute(pathname);
var { shallow = false } = options;
+ var cleanedAs = delBasePath(as);
if ((0, _isDynamic.isDynamicRoute)(route)) {
- var { pathname: asPathname } = (0, _url.parse)(as);
+ var { pathname: asPathname } = (0, _url.parse)(cleanedAs);
var routeRegex = (0, _routeRegex.getRouteRegex)(route);
var routeMatch = (0, _routeMatcher.getRouteMatcher)(routeRegex)(
- delBasePath(asPathname || "")
+ asPathname
);
if (!routeMatch) {
@@ -1040,21 +1043,17 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
var appComp;
}
- this.set(
- route,
- pathname,
- query,
- delBasePath(as),
- routeInfo
- ).then(() => {
- if (error) {
- Router.events.emit("routeChangeError", error, as);
- throw error;
- }
+ this.set(route, pathname, query, cleanedAs, routeInfo).then(
+ () => {
+ if (error) {
+ Router.events.emit("routeChangeError", error, as);
+ throw error;
+ }
- Router.events.emit("routeChangeComplete", as);
- return resolve(true);
- });
+ Router.events.emit("routeChangeComplete", as);
+ return resolve(true);
+ }
+ );
},
reject
);Diff for index.html
@@ -24,7 +24,7 @@
/>
<link
rel="preload"
- href="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.deb50154f80a4044eb84.module.js"
+ href="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.6b23cf48f915c17ee2a9.module.js"
as="script"
crossorigin="anonymous"
/>
@@ -117,13 +117,13 @@
type="module"
></script>
<script
- src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.c055dea44cbce095b7e0.js"
+ src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.23a257d02f3edb6832d1.js"
async=""
crossorigin="anonymous"
nomodule=""
></script>
<script
- src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.deb50154f80a4044eb84.module.js"
+ src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.6b23cf48f915c17ee2a9.module.js"
async=""
crossorigin="anonymous"
type="module"Diff for link.html
@@ -24,7 +24,7 @@
/>
<link
rel="preload"
- href="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.deb50154f80a4044eb84.module.js"
+ href="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.6b23cf48f915c17ee2a9.module.js"
as="script"
crossorigin="anonymous"
/>
@@ -122,13 +122,13 @@
type="module"
></script>
<script
- src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.c055dea44cbce095b7e0.js"
+ src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.23a257d02f3edb6832d1.js"
async=""
crossorigin="anonymous"
nomodule=""
></script>
<script
- src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.deb50154f80a4044eb84.module.js"
+ src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.6b23cf48f915c17ee2a9.module.js"
async=""
crossorigin="anonymous"
type="module"Diff for withRouter.html
@@ -24,7 +24,7 @@
/>
<link
rel="preload"
- href="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.deb50154f80a4044eb84.module.js"
+ href="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.6b23cf48f915c17ee2a9.module.js"
as="script"
crossorigin="anonymous"
/>
@@ -117,13 +117,13 @@
type="module"
></script>
<script
- src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.c055dea44cbce095b7e0.js"
+ src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.23a257d02f3edb6832d1.js"
async=""
crossorigin="anonymous"
nomodule=""
></script>
<script
- src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.deb50154f80a4044eb84.module.js"
+ src="/_next/static/chunks/de003c3a9d308750aa009870a5926f9b18ab31f4.6b23cf48f915c17ee2a9.module.js"
async=""
crossorigin="anonymous"
type="module"Serverless Mode (Increase detected ⚠️ )
General Overall increase ⚠️
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| buildDuration | 12.7s | 12.6s | -99ms |
| nodeModulesSize | 67 MB | 67 MB |
Client Bundles (main, webpack, commons) Overall increase ⚠️
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| main-HASH.js gzip | 6.51 kB | 6.51 kB | ✓ |
| webpack-HASH.js gzip | 746 B | 746 B | ✓ |
| de003c3a9d30..4dd5.js gzip | 10.5 kB | N/A | N/A |
| framework.HASH.js gzip | 39.1 kB | 39.1 kB | ✓ |
| de003c3a9d30..75a9.js gzip | N/A | 10.5 kB | N/A |
| Overall change | 56.9 kB | 56.9 kB |
Client Bundles (main, webpack, commons) Modern Overall decrease ✓
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| main-HASH.module.js gzip | 5.6 kB | 5.6 kB | ✓ |
| webpack-HASH..dule.js gzip | 746 B | 746 B | ✓ |
| de003c3a9d30..dule.js gzip | 6.92 kB | N/A | N/A |
| framework.HA..dule.js gzip | 39.1 kB | 39.1 kB | ✓ |
| de003c3a9d30..dule.js gzip | N/A | 6.92 kB | N/A |
| Overall change | 52.4 kB | 52.4 kB | -2 B |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 26.3 kB | 26.3 kB | ✓ |
| Overall change | 26.3 kB | 26.3 kB | ✓ |
Client Pages
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| routerDirect.js gzip | 279 B | 279 B | ✓ |
| withRouter.js gzip | 278 B | 278 B | ✓ |
| _error.js gzip | 3.37 kB | 3.37 kB | ✓ |
| index.js gzip | 222 B | 222 B | ✓ |
| link.js gzip | 2.05 kB | 2.05 kB | ✓ |
| hooks.js gzip | 881 B | 881 B | ✓ |
| _app.js gzip | 1.26 kB | 1.26 kB | ✓ |
| Overall change | 8.34 kB | 8.34 kB | ✓ |
Client Pages Modern
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| index.module.js gzip | 223 B | 223 B | ✓ |
| routerDirect..dule.js gzip | 281 B | 281 B | ✓ |
| withRouter.m..dule.js gzip | 278 B | 278 B | ✓ |
| hooks.module.js gzip | 383 B | 383 B | ✓ |
| _error.module.js gzip | 2.21 kB | 2.21 kB | ✓ |
| link.module.js gzip | 1.52 kB | 1.52 kB | ✓ |
| _app.module.js gzip | 604 B | 604 B | ✓ |
| Overall change | 5.49 kB | 5.49 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 270 B | 270 B | ✓ |
| _buildManife..dule.js gzip | 274 B | 274 B | ✓ |
| Overall change | 544 B | 544 B | ✓ |
Serverless bundles Overall increase ⚠️
| vercel/next.js canary | ijjk/next.js fix/basepath-index-gsp | Change | |
|---|---|---|---|
| _error.js | 875 kB | 875 kB | ✓ |
| 404.html | 4.17 kB | 4.17 kB | ✓ |
| hooks.html | 3.79 kB | 3.79 kB | ✓ |
| index.js | 875 kB | 875 kB | ✓ |
| link.js | 914 kB | 914 kB | |
| routerDirect.js | 906 kB | 906 kB | |
| withRouter.js | 906 kB | 906 kB | |
| Overall change | 4.48 MB | 4.48 MB |
kodiakhq bot
pushed a commit
that referenced
this pull request
Jun 23, 2020
Noticed this while reviewing #14376. After having done #13699, this code didn't feel right to me: ```js function prepareRoute(path: string) { path = delBasePath(path || '') // this /index rewrite is problematic, it makes pages/index.js // and pages/index/index.js point to the same thing: return toRoute(!path || path === '/' ? '/index' : path) } ``` Added a nested index page to the prerender tests and found it was rendering the `/` route on navigation. This uncovered 2 more places around the dataroute where the index path was not translated correctly. **edit:** Just to note that there was nothing wrong with #14376, the issue was already there, I just noticed it while reading that PR
rokinsky
pushed a commit
to rokinsky/next.js
that referenced
this pull request
Jul 11, 2020
This corrects the `/_next/data` path generated when using `basePath` with `getStaticProps` in a `pages/index.js` file which was previously stripping the `basePath` without checking if `/index` needed to be appended after stripping. This also adds additional checks to the `basePath` test suite to prevent regressing x-ref: vercel#9872 (comment)
rokinsky
pushed a commit
to rokinsky/next.js
that referenced
this pull request
Jul 11, 2020
Noticed this while reviewing vercel#14376. After having done vercel#13699, this code didn't feel right to me: ```js function prepareRoute(path: string) { path = delBasePath(path || '') // this /index rewrite is problematic, it makes pages/index.js // and pages/index/index.js point to the same thing: return toRoute(!path || path === '/' ? '/index' : path) } ``` Added a nested index page to the prerender tests and found it was rendering the `/` route on navigation. This uncovered 2 more places around the dataroute where the index path was not translated correctly. **edit:** Just to note that there was nothing wrong with vercel#14376, the issue was already there, I just noticed it while reading that PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This corrects the
/_next/datapath generated when usingbasePathwithgetStaticPropsin apages/index.jsfile which was previously stripping thebasePathwithout checking if/indexneeded to be appended after stripping. This also adds additional checks to thebasePathtest suite to prevent regressingx-ref: #9872 (comment)