Skip to content
This repository was archived by the owner on Jan 28, 2025. It is now read-only.

Commit f60ff4a

Browse files
dphangdphang
authored
fix(core): fix 'basePath: false' external rewrites (#1859)
1 parent 4316b18 commit f60ff4a

File tree

8 files changed

+174
-105
lines changed

8 files changed

+174
-105
lines changed

‎packages/e2e-tests/next-app-with-base-path/cypress/integration/rewrites.test.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,5 +80,42 @@ describe("Rewrites Tests", () => {
8080
}
8181
});
8282
});
83+
84+
[
85+
{
86+
path: "/basepath/external-rewrite",
87+
expectedRewrite: "https://jsonplaceholder.typicode.com/users",
88+
method: "GET",
89+
expectedStatus: 200,
90+
body: undefined
91+
},
92+
{
93+
path: "/external-rewrite-without-basepath",
94+
expectedRewrite: "https://jsonplaceholder.typicode.com/users",
95+
method: "GET",
96+
expectedStatus: 200,
97+
body: undefined
98+
}
99+
].forEach(({ path, expectedRewrite, method, body, expectedStatus }) => {
100+
it(`externally rewrites path ${path} to ${expectedRewrite} for method ${method}`, () => {
101+
cy.request({
102+
url: path,
103+
method,
104+
body,
105+
failOnStatusCode: false
106+
}).then((response) => {
107+
expect(response.status).to.equal(expectedStatus);
108+
cy.request({
109+
url: expectedRewrite,
110+
method,
111+
body,
112+
failOnStatusCode: false
113+
}).then((rewriteResponse) => {
114+
// Check that the body of each page is the same, i.e it is actually rewritten
115+
expect(response.body).to.deep.equal(rewriteResponse.body);
116+
});
117+
});
118+
});
119+
});
83120
});
84121
});

‎packages/e2e-tests/next-app-with-base-path/next.config.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,15 @@ module.exports = {
6969
},
7070
async rewrites() {
7171
return [
72+
{
73+
source: "/external-rewrite",
74+
destination: "https://jsonplaceholder.typicode.com/users"
75+
},
76+
{
77+
source: "/external-rewrite-without-basepath",
78+
destination: "https://jsonplaceholder.typicode.com/users",
79+
basePath: false
80+
},
7281
{
7382
source: "/rewrite",
7483
destination: "/ssr-page"

‎packages/e2e-tests/next-app/cypress/integration/rewrites.test.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -151,25 +151,19 @@ describe("Rewrites Tests", () => {
151151
expectedStatus: 200
152152
}
153153
].forEach(({ path, expectedRewrite, method, body, expectedStatus }) => {
154-
const headers = Cypress.env("GITHUB_TOKEN")
155-
? { Authorization: `Bearer ${Cypress.env("GITHUB_TOKEN")}` }
156-
: undefined;
157-
158154
it(`externally rewrites path ${path} to ${expectedRewrite} for method ${method}`, () => {
159155
cy.request({
160156
url: path,
161157
method: method,
162158
body: body,
163-
failOnStatusCode: false,
164-
headers: headers
159+
failOnStatusCode: false
165160
}).then((response) => {
166161
expect(response.status).to.equal(expectedStatus);
167162
cy.request({
168163
url: expectedRewrite,
169164
method: method,
170165
body: body,
171-
failOnStatusCode: false,
172-
headers: headers
166+
failOnStatusCode: false
173167
}).then((rewriteResponse) => {
174168
// Check that the body of each page is the same, i.e it is actually rewritten
175169
expect(response.body).to.deep.equal(rewriteResponse.body);

‎packages/libs/core/src/route/api.ts‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,19 @@ export const handleApiReq = (
1818
isRewrite?: boolean
1919
): ExternalRoute | ApiRoute | undefined => {
2020
const { apis } = manifest;
21-
const normalisedUri = normalise(uri, routesManifest);
21+
const { normalisedUri, missingExpectedBasePath } = normalise(
22+
uri,
23+
routesManifest
24+
);
2225

23-
const nonDynamic = apis.nonDynamic[normalisedUri];
24-
if (nonDynamic) {
25-
return {
26-
isApi: true,
27-
page: nonDynamic
28-
};
26+
if (!missingExpectedBasePath) {
27+
const nonDynamic = apis.nonDynamic[normalisedUri];
28+
if (nonDynamic) {
29+
return {
30+
isApi: true,
31+
page: nonDynamic
32+
};
33+
}
2934
}
3035

3136
const rewrite = !isRewrite && getRewritePath(req, uri, routesManifest);
@@ -50,11 +55,13 @@ export const handleApiReq = (
5055
return route;
5156
}
5257

53-
const dynamic = matchDynamic(normalisedUri, apis.dynamic);
54-
if (dynamic) {
55-
return {
56-
isApi: true,
57-
page: dynamic
58-
};
58+
if (!missingExpectedBasePath) {
59+
const dynamic = matchDynamic(normalisedUri, apis.dynamic);
60+
if (dynamic) {
61+
return {
62+
isApi: true,
63+
page: dynamic
64+
};
65+
}
5966
}
6067
};

‎packages/libs/core/src/route/basepath.ts‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,15 @@ import { RoutesManifest } from "../types";
33
export const normalise = (
44
uri: string,
55
routesManifest: RoutesManifest
6-
): string => {
7-
const { basePath, i18n } = routesManifest;
6+
): { normalisedUri: string; missingExpectedBasePath: boolean } => {
7+
const { basePath } = routesManifest;
88
if (basePath) {
99
if (uri.startsWith(basePath)) {
1010
uri = uri.slice(basePath.length);
1111
} else {
12-
// basePath set but URI does not start with basePath, return 404
13-
if (i18n?.defaultLocale) {
14-
return `/${i18n.defaultLocale}/404`;
15-
} else {
16-
return "/404";
17-
}
12+
// basePath set but URI does not start with basePath, return original path with special flag indicating missing expected base path
13+
// but basePath is expected
14+
return { normalisedUri: uri, missingExpectedBasePath: true };
1815
}
1916
}
2017

@@ -24,5 +21,8 @@ export const normalise = (
2421
}
2522

2623
// Empty path should be normalised to "/" as there is no Next.js route for ""
27-
return uri === "" ? "/" : uri;
24+
return {
25+
normalisedUri: uri === "" ? "/" : uri,
26+
missingExpectedBasePath: false
27+
};
2828
};

‎packages/libs/core/src/route/index.ts‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -150,20 +150,27 @@ export const routeDefault = async (
150150
return domainRedirect;
151151
}
152152

153-
const uri = normalise(req.uri, routesManifest);
153+
const { normalisedUri: uri, missingExpectedBasePath } = normalise(
154+
req.uri,
155+
routesManifest
156+
);
154157
const is404 = uri.endsWith("/404");
155158
const isDataReq = uri.startsWith("/_next/data");
156159
const publicFile = handlePublicFiles(uri, manifest);
157160
const isPublicFile = !!publicFile;
158161

159-
const trailingSlash =
160-
!is404 && handleTrailingSlash(req, manifest, isDataReq || isPublicFile);
161-
if (trailingSlash) {
162-
return trailingSlash;
163-
}
164-
165-
if (publicFile) {
166-
return publicFile;
162+
// Only try to handle trailing slash redirects or public files if the URI isn't missing a base path.
163+
// This allows us to handle redirects without base paths.
164+
if (!missingExpectedBasePath) {
165+
const trailingSlash =
166+
!is404 && handleTrailingSlash(req, manifest, isDataReq || isPublicFile);
167+
if (trailingSlash) {
168+
return trailingSlash;
169+
}
170+
171+
if (publicFile) {
172+
return publicFile;
173+
}
167174
}
168175

169176
const otherRedirect =

‎packages/libs/core/src/route/page.ts‎

Lines changed: 80 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -32,57 +32,69 @@ export const handlePageReq = (
3232
isRewrite?: boolean
3333
): ExternalRoute | PageRoute | ApiRoute => {
3434
const { pages } = manifest;
35-
const localeUri = normalise(
35+
const { normalisedUri: localeUri, missingExpectedBasePath } = normalise(
3636
addDefaultLocaleToPath(
3737
uri,
3838
routesManifest,
3939
findDomainLocale(req, routesManifest)
4040
),
4141
routesManifest
4242
);
43-
if (pages.html.nonDynamic[localeUri]) {
44-
const nonLocaleUri = dropLocaleFromPath(localeUri, routesManifest);
45-
const statusCode =
46-
nonLocaleUri === "/404" ? 404 : nonLocaleUri === "/500" ? 500 : undefined;
47-
return {
48-
isData: false,
49-
isStatic: true,
50-
file: pages.html.nonDynamic[localeUri],
51-
statusCode
52-
};
53-
}
54-
if (pages.ssg.nonDynamic[localeUri] && !isPreview) {
55-
const ssg = pages.ssg.nonDynamic[localeUri];
56-
const route = ssg.srcRoute ?? localeUri;
57-
const nonLocaleUri = dropLocaleFromPath(localeUri, routesManifest);
58-
const statusCode =
59-
nonLocaleUri === "/404" ? 404 : nonLocaleUri === "/500" ? 500 : undefined;
60-
return {
61-
isData: false,
62-
isStatic: true,
63-
file: pageHtml(localeUri),
64-
// page JS path is from SSR entries in manifest
65-
page: pages.ssr.nonDynamic[route] || pages.ssr.dynamic[route],
66-
revalidate: ssg.initialRevalidateSeconds,
67-
statusCode
68-
};
69-
}
70-
if ((pages.ssg.notFound ?? {})[localeUri] && !isPreview) {
71-
return notFoundPage(uri, manifest, routesManifest);
72-
}
73-
if (pages.ssr.nonDynamic[localeUri]) {
74-
if (localeUri.startsWith("/api/")) {
43+
44+
// This allows matching against rewrites even without basepath
45+
if (!missingExpectedBasePath) {
46+
if (pages.html.nonDynamic[localeUri]) {
47+
const nonLocaleUri = dropLocaleFromPath(localeUri, routesManifest);
48+
const statusCode =
49+
nonLocaleUri === "/404"
50+
? 404
51+
: nonLocaleUri === "/500"
52+
? 500
53+
: undefined;
7554
return {
76-
isApi: true,
77-
page: pages.ssr.nonDynamic[localeUri]
55+
isData: false,
56+
isStatic: true,
57+
file: pages.html.nonDynamic[localeUri],
58+
statusCode
7859
};
79-
} else {
60+
}
61+
if (pages.ssg.nonDynamic[localeUri] && !isPreview) {
62+
const ssg = pages.ssg.nonDynamic[localeUri];
63+
const route = ssg.srcRoute ?? localeUri;
64+
const nonLocaleUri = dropLocaleFromPath(localeUri, routesManifest);
65+
const statusCode =
66+
nonLocaleUri === "/404"
67+
? 404
68+
: nonLocaleUri === "/500"
69+
? 500
70+
: undefined;
8071
return {
8172
isData: false,
82-
isRender: true,
83-
page: pages.ssr.nonDynamic[localeUri]
73+
isStatic: true,
74+
file: pageHtml(localeUri),
75+
// page JS path is from SSR entries in manifest
76+
page: pages.ssr.nonDynamic[route] || pages.ssr.dynamic[route],
77+
revalidate: ssg.initialRevalidateSeconds,
78+
statusCode
8479
};
8580
}
81+
if ((pages.ssg.notFound ?? {})[localeUri] && !isPreview) {
82+
return notFoundPage(uri, manifest, routesManifest);
83+
}
84+
if (pages.ssr.nonDynamic[localeUri]) {
85+
if (localeUri.startsWith("/api/")) {
86+
return {
87+
isApi: true,
88+
page: pages.ssr.nonDynamic[localeUri]
89+
};
90+
} else {
91+
return {
92+
isData: false,
93+
isRender: true,
94+
page: pages.ssr.nonDynamic[localeUri]
95+
};
96+
}
97+
}
8698
}
8799

88100
const rewrite =
@@ -110,41 +122,44 @@ export const handlePageReq = (
110122
};
111123
}
112124

113-
const dynamic = matchDynamicRoute(localeUri, pages.dynamic);
125+
// We don't want to match URIs with missing basepath against dynamic routes if it wasn't already covered by rewrite.
126+
if (!missingExpectedBasePath) {
127+
const dynamic = matchDynamicRoute(localeUri, pages.dynamic);
114128

115-
const dynamicSSG = dynamic && pages.ssg.dynamic[dynamic];
116-
if (dynamicSSG && !isPreview) {
117-
return {
118-
isData: false,
119-
isStatic: true,
120-
file: pageHtml(localeUri),
121-
page: dynamic ? pages.ssr.dynamic[dynamic] : undefined, // page JS path is from SSR entries in manifest
122-
fallback: dynamicSSG.fallback
123-
};
124-
}
125-
const dynamicSSR = dynamic && pages.ssr.dynamic[dynamic];
126-
if (dynamicSSR) {
127-
if (dynamic.startsWith("/api/")) {
129+
const dynamicSSG = dynamic && pages.ssg.dynamic[dynamic];
130+
if (dynamicSSG && !isPreview) {
128131
return {
129-
isApi: true,
130-
page: dynamicSSR
132+
isData: false,
133+
isStatic: true,
134+
file: pageHtml(localeUri),
135+
page: dynamic ? pages.ssr.dynamic[dynamic] : undefined, // page JS path is from SSR entries in manifest
136+
fallback: dynamicSSG.fallback
131137
};
132-
} else {
138+
}
139+
const dynamicSSR = dynamic && pages.ssr.dynamic[dynamic];
140+
if (dynamicSSR) {
141+
if (dynamic.startsWith("/api/")) {
142+
return {
143+
isApi: true,
144+
page: dynamicSSR
145+
};
146+
} else {
147+
return {
148+
isData: false,
149+
isRender: true,
150+
page: dynamicSSR
151+
};
152+
}
153+
}
154+
const dynamicHTML = dynamic && pages.html.dynamic[dynamic];
155+
if (dynamicHTML) {
133156
return {
134157
isData: false,
135-
isRender: true,
136-
page: dynamicSSR
158+
isStatic: true,
159+
file: dynamicHTML
137160
};
138161
}
139162
}
140-
const dynamicHTML = dynamic && pages.html.dynamic[dynamic];
141-
if (dynamicHTML) {
142-
return {
143-
isData: false,
144-
isStatic: true,
145-
file: dynamicHTML
146-
};
147-
}
148163

149164
return notFoundPage(uri, manifest, routesManifest);
150165
};

‎packages/libs/lambda-at-edge/tests/default-handler/default-handler-with-basepath.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ describe("Lambda@Edge", () => {
341341
${"/manifest.json/"}
342342
`(
343343
"serves 404 page from S3 for path without basepath prefix: $path",
344-
async ({ path, expectedPage }) => {
344+
async ({ path }) => {
345345
const event = createCloudFrontEvent({
346346
uri: path,
347347
host: "mydistribution.cloudfront.net"

0 commit comments

Comments
 (0)