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

fix: request path mismatch in RSC payload that occurred during static export with basePath set. #73912

Open
wants to merge 4 commits into
base: canary
Choose a base branch
from

Conversation

@ijjk
Copy link
Member

ijjk commented Dec 13, 2024

Allow CI Workflow Run

  • approve CI run for commit: 15cde9a

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@Hartaithan
Copy link

@acdlite @unstubbable
Can someone take a look at this?
This PR might solve the problem with #73427

@klittlepage
Copy link

Thank you, @yukiyokotani. I came across this PR when hunting down a related bug resulting from the use of skipTrailingSlashRedirect that doesn't appear to have an open issue. The patch below addresses it. I'm happy to open a PR against your repo, or if you want to update this PR directly, go for it.

diff --git a/packages/next/src/client/components/router-reducer/fetch-server-response.ts b/packages/next/src/client/components/router-reducer/fetch-server-response.ts
index 02545c4ddb..7b25952dec 100644
--- a/packages/next/src/client/components/router-reducer/fetch-server-response.ts
+++ b/packages/next/src/client/components/router-reducer/fetch-server-response.ts
@@ -11,6 +11,8 @@ const { createFromReadableStream } = (
       require('react-server-dom-webpack/client')
 ) as typeof import('react-server-dom-webpack/client')
 const basePath = (process.env.__NEXT_ROUTER_BASEPATH as string) || ''
+const skipTrailingSlashRedirect =
+  (process.env.__NEXT_MANUAL_TRAILING_SLASH as boolean) ?? false

 import type {
   FlightRouterState,
@@ -230,7 +232,7 @@ export function createFetch(

   if (process.env.NODE_ENV === 'production') {
     if (process.env.__NEXT_CONFIG_OUTPUT === 'export') {
-      if (fetchUrl.pathname === basePath) {
+      if (fetchUrl.pathname === basePath || skipTrailingSlashRedirect) {
         fetchUrl.pathname += '/index.txt'
       } else if (fetchUrl.pathname.endsWith('/')) {
         fetchUrl.pathname += 'index.txt'

@yukiyokotani
Copy link
Author

@klittlepage Happy New Year! Thank you for finding this issue and pointing out the additional fixes. I was able to reproduce the issue on my end and confirm that the proposed code changes resolve it. I have updated the PR accordingly.

@klittlepage
Copy link

Thank you @yukiyokotani! And Happy New Year to you as well.

@klittlepage
Copy link

@ijjk, is there anything else to do here, or can we get this approved for a CI run? This bug is a show-stopper for static export in some deployment scenarios.

@@ -256,7 +259,9 @@ export function createFetch(

if (process.env.NODE_ENV === 'production') {
if (process.env.__NEXT_CONFIG_OUTPUT === 'export') {
if (fetchUrl.pathname.endsWith('/')) {
if (fetchUrl.pathname === basePath || skipTrailingSlashRedirect) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's correct to always add if skipTrailingSlashRedirect is configured as this is more specific to where the file would be located which depends on the trailingSlash config https://github.com/vercel/next.js/blob/canary/packages/next/src/export/index.ts#L96

Copy link
Author

Choose a reason for hiding this comment

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

I thought the file name was correct and the request path was incorrect, but could it actually be the other way around? That is, the request path is correct, and the file name is incorrect? The way to fix this will vary depending on the policy.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants