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

SSR-prefetches with middleware break application between deployments #59612

Closed
1 task done
thoaltmann opened this issue Dec 14, 2023 · 17 comments
Closed
1 task done

SSR-prefetches with middleware break application between deployments #59612

thoaltmann opened this issue Dec 14, 2023 · 17 comments
Labels
bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation. Pages Router Related to Pages Router. Runtime Related to Node.js or Edge Runtime with Next.js.

Comments

@thoaltmann
Copy link

Link to the code that reproduces this issue

https://github.com/thoaltmann/next-middleware-prefetch

To Reproduce

  1. Build and serve the application the first time
  2. Visit the index page
  3. While keeping the page open, rebuild and serve the application without making any changes
  4. Click on a link on the index page

Current vs. Expected behavior

Current Behavior:

When a middleware is present, hovering over links to pages with getServerSideProps, the client sends _next/data requests (which is the intended behavior and that's okay). After the first build, the response also includes Headers to signal the client that the request was skipped/bailed (X-Middleware-Skip=1) so the actual request is then made when the user clicks on the link.

However, after the app has been rebuilt/served/deployed, this behavior changes. The _next/data request then also responds with an empty object, but the X-Middleware-Skip header is missing. Instead, a resolved path with the previous build-id is in the X-Nextjs-Matched-Path Header (e.g. /_next/data/5NLSryPAF39BoEpy7KklK/test/1.json). With this response, the client application doesn't know that this was a bailed SSR-request and uses the empty object as the actual data for the page, resulting in an error. When using a cache, this problem persists as long as the cache does, as the rendered HTML contains an old build-id in the __NEXT_DATA__-script

Expected Behavior:

After the second build, the _next/data-response should include the necessary headers for the client, so it doesn't use the empty object as actual data. After clicking a link and making the actual request to the data, the server will then send a 404, which leads to a hard navigation, similarly to how it behaves without a middleware.

next-middleware-prefetch

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 21.1.0: Wed Oct 13 17:33:01 PDT 2021; root:xnu-8019.41.5~1/RELEASE_ARM64_T6000
Binaries:
  Node: 18.17.1
  npm: 9.6.7
  Yarn: 1.22.19
  pnpm: 8.10.4
Relevant Packages:
  next: 14.0.5-canary.12
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.1.3
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Data fetching (gS(S)P, getInitialProps), Middleware / Edge (API routes, runtime), Routing (next/router, next/navigation, next/link)

Additional context

No response

@thoaltmann thoaltmann added the bug Issue was opened via the bug report template. label Dec 14, 2023
@github-actions github-actions bot added Pages Router Related to Pages Router. Runtime Related to Node.js or Edge Runtime with Next.js. Navigation Related to Next.js linking (e.g., <Link>) and navigation. labels Dec 14, 2023
@vilindberg
Copy link

@thoaltmann Thanks for creating this issue. I am experiencing the same problem. Did you find any temporary solution to the problem?

I don't think it is only related to prefetches, since it doesn't work when doing a router.push either (after a re-build). After a bit of investigation, I think that this behaviour was introduces in version 13.4.13-canary.0. It work fine in version 13.4.12.

@vilindberg
Copy link

The commit that seems to have created the issue (1398de9) unfortunately involves a significant rework/restructure of the routing logic which makes it a bit hard for a newcomer of the repo to understand the full scope of the changes.

However, I suspect the cause of the issue is this if-statement:

Removing this if-statement seems to resolve the issue, reverting the behavior to its previous state. After this modification, a 404 error on the _next/data/the-build-id/page.json-request is followed by a 200 on the /page-request. Additionally, running the test suite post-modification shows no issues.

Sorry to bother you @ijjk , but I was wondering if you might have any insights into the rationale behind this if-statement's inclusion. Understanding that its removal might impact other functionalities, I'd greatly appreciate any guidance on potential alternative solutions or areas to investigate further.

@thoaltmann
Copy link
Author

Hi @vilindberg, unfortunately we didn't find a solution and had to create our own custom middleware as a cloudfront viewer request lambda.

@vilindberg
Copy link

@thoaltmann FYI. I think I have found a workaround for this issue. That this workaround works would also explain why there are not a gazillion issues created about this.

Workaround:
Adding an app-folder with:

// app/layout.tsx - A default layout
export default function RootLayout({ children }: { children: React.ReactNode }) {
  return (
    <html lang="en">
      <body>{children}</body>
    </html>
  );
}

// app/whatever/page.tsx - Dummy app page
export default function Page() {
  return null;
}

Seems to resolve the issue. On hover you get a 404 and then on click you get a hard navigation resulting in a 200 with the document.

@ydubinskyi
Copy link

@vilindberg thanks, it worked for me too!

@ydubinskyi
Copy link

ydubinskyi commented Jan 10, 2024

The commit that seems to have created the issue (1398de9) unfortunately involves a significant rework/restructure of the routing logic which makes it a bit hard for a newcomer of the repo to understand the full scope of the changes.

However, I suspect the cause of the issue is this if-statement:

Removing this if-statement seems to resolve the issue, reverting the behavior to its previous state. After this modification, a 404 error on the _next/data/the-build-id/page.json-request is followed by a 200 on the /page-request. Additionally, running the test suite post-modification shows no issues.

Sorry to bother you @ijjk , but I was wondering if you might have any insights into the rationale behind this if-statement's inclusion. Understanding that its removal might impact other functionalities, I'd greatly appreciate any guidance on potential alternative solutions or areas to investigate further.

Maybe we do not need to remove entire if statement, but just replace status code with 404 instead of 200, for me it fixed a problem during debugging.

@vilindberg
Copy link

@ydubinskyi Yeah, maybe it is better to do an early null-return instead. In that case the addition of the "x-nextjs-matched-path"-header should probably be removed as well, since it includes a path with the previous buildId.

@karaoak
Copy link

karaoak commented Jan 17, 2024

@vilindberg Your fix works.

However, for it to work I also need to annotate the layout and pages with use client. Also Nextjs forces me to add a _not-found/page to my app folder. This takes precedence over the pages folder 404.tsx page. So now our application is underwater serving a 404 and in the know about changed buildId. However, the application is first of all generation a lot of unnecessary 404 errors, but more importantly serving a wrong 404 page for unknown routes. And since we rely on getServeriSideProps pages, also for the 404 pages page. This solution unfortunately won't work for us.

@vilindberg
Copy link

@karaoak Interesting! Do you have a small repro of it not working without use client?

Yeah, all the 404 errors are kind of irritating. When preserving logs, the solutions above (and also the pre v13.4.13 behaviour) causes the following 404's before the hard navigation.

First 404 - Request headers:
X-Middleware-Prefetch: 1
X-Nextjs-Data: 1

Second 404 - Request headers:
X-Nextjs-Data: 1

Third 404 - Request headers:
X-Nextjs-Data: 1

All 404's are to the _next/data/old-build-id/page.json-route.

In my uninformed opinion, if you get a path miss, but the miss is only due to a buildId-mismatch, the server should instantly return a 404 to the client with a header or whatever to tell the client to do a hard navigation.

@karaoak
Copy link

karaoak commented Jan 18, 2024

@vilindberg No, I'm sorry, unfortunately I don't have that available in a small repo.
Problem was, Next.js 14.0.4 forced to also add an src/app/_not_found/page.tsx page. I might try to look into ways not having to do this, cause then our regular 404 page will be served instead.
But Ideally we would not have to hotfix stuff like this. Cause Ideally I would not like to have an app routing folder in the project I epxerience this issue with.

@karaoak
Copy link

karaoak commented Jan 19, 2024

Unfortunately also not fixed with the release of Next 14.1.0 😢

@karaoak
Copy link

karaoak commented Jan 19, 2024

I was able to fix our issue, either by removing our custom middleware.ts (not really an option), or to change it a bit.
Cause I did not wanted to add app routing to our app, this would also increase our bundle size, it's not required and above all was unwanted with regard to our 404 pages (see my comment above).

src/middleware.ts

const buildIdRegex = /\/_next\/data\/([^\/]+)/;

export const middleware = (request: NextRequest): NextResponse => {
    const host = request.headers.get('host');

    const response = NextResponse.next();
    if (request.nextUrl.pathname.startsWith('/_next/data/')) {
        const matches = request.nextUrl.pathname.match(buildIdRegex);
        if (matches?.length > 1 && matches[1] !== process.env.NEXT_BUILD_ID) {
            return NextResponse.redirect(new URL('/404', request.url));
        }
    }

   // .... we dome very secret custom stuff here ;-)
  return response;
 } 

And for this to work and be able to grab the buildId from the _next/data/<buildId> xhr requests, I needed to add this to my next.config.js.

As well, make the Next BUILD_ID available at runtime, so that I can actually perform the check in above snippet.

next.config.js

skipMiddlewareUrlNormalize: true,
webpack: (config, { buildId, isServer, webpack }) => {
    config.plugins.push(
      new DefinePlugin({
          'process.env.NEXT_BUILD_ID': JSON.stringify(buildId),

See: Advanced Middleware Flags for the skipMiddlewareUrlNormalize config option.

@Lezzio
Copy link
Contributor

Lezzio commented Jan 21, 2024

I found this solution to work in our case:
This requires in next.config.js:

  • the DefinePlugin to replace the BUILD_ID variable with Next.js' buildId
  • skipMiddlewareUrlNormalize set to true

This helps the client to eventually force refresh its JavaScript and aligns the version again for smooth experience and client-server communication

export const middleware = (req: NextRequest): NextResponse | void => {
  if (req.nextUrl.buildId && req.nextUrl.buildId !== process.env.BUILD_ID) {

    return NextResponse.json(
      { message: 'client version outdated, propagate exception to the client to force refresh' },
      { status: 500 }
    )
  }
 // ...
}

Since this is a version skew problem, there is a few options among:

  • Backward compatibility, which is isn't supported by the default build system
  • Force the client to update/refresh which seems to work with the snippet above (let's improve it ?)

@karaoak
Copy link

karaoak commented Jan 22, 2024

@Lezzio Nice, I see request.nextUrl.buildId also becomes available when enabling skipMiddlewareUrlNormalize 🥳

@ben-propflo
Copy link

ben-propflo commented Feb 3, 2024

Pages with middleware has been borked for a while, here's something related I reported back in October: #57207 and there's a bunch more unresolved issues here: #56222 (comment)

Disappointing that these have been open for a while and don't seem to be getting any attention from the devs.

@ijjk
Copy link
Member

ijjk commented Feb 6, 2024

Closing as this should be resolved by #60968, please upgrade to the latest canary of Next.js and give it a try!

Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation. Pages Router Related to Pages Router. Runtime Related to Node.js or Edge Runtime with Next.js.
Projects
None yet
Development

No branches or pull requests

7 participants