vercel / next.js

The React Framework
https://nextjs.org
MIT License
124.19k stars 26.47k forks source link

Click the browser forward and back, and getServerSideProps does not re-execute #67763

Open rick-liruixin opened 1 month ago

rick-liruixin commented 1 month ago

Link to the code that reproduces this issue

https://github.com/rick-liruixin/next-getServerSideProps-bug-demo

To Reproduce

Page: categories/[...url].tsx

When navigating from https://www.alexandermcqueen.cn/categories/women/ready-to-wear.html to https://www.alexandermcqueen.cn/categories/women.html, and then to https://www.alexandermcqueen.cn/categories/women/shoes.html,

Clicking the browser's forward and backward buttons to move between these pages sometimes causes getServerSideProps not to trigger.

The code is as follows:

export const getServerSideProps: GetServerSideProps<
  ServerSidePropsWithStatus,
  { url: Array<string> }
> = async ({ query, req: serverReq, res: serverRes }) => {
  const serverContext = { req: serverReq, res: serverRes }
  const paramsData = {
    q: '',
    page: 1,
    ...query,
    isAll: true,
  }
    const { data: res } = await api.search.getSearchResultWithLayer(
      paramsData,
      serverContext,
    )
    const initialQueryData = paramsData
    return {
      props: { q: query.q, initialQueryData, ...res.data },
    }
}

Current vs. Expected behavior

Even though it's a single page, getServerSideProps should trigger every time we navigate forward and backward.

Provide environment information

Operating System:
  Platform: darwin
  Arch: x64
  Version: Darwin Kernel Version 22.4.0: Mon Mar  6 21:00:17 PST 2023; root:xnu-8796.101.5~3/RELEASE_X86_64
Binaries:
  Node: 18.12.1
  npm: 8.19.2
  Yarn: 1.22.19
  pnpm: 8.2.0
Relevant Packages:
  next: 13.5.6
  eslint-config-next: 13.5.6
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.0.2
Next.js Config:
  output: standalone

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

Not sure

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

next start (local), Vercel (Deployed)

Additional context

Local recurrence rates are low, so I've listed online environments that can be accessed to see the problem

rick-liruixin commented 1 month ago

Do not directly click links or change the address bar to navigate. Use the internal menu on the page to navigate to reproduce the issue.

icyJoseph commented 1 month ago

I can't reproduce... could you take a video snippet of the problem?

rick-liruixin commented 1 month ago

https://github.com/user-attachments/assets/871f51cd-db74-481c-a97d-faccc6801170

this is wicked, Each time we switch pages, a .json request should be sent, but it isn't. Initially, I thought it was because the DOM wasn't updating, so I dynamically updated a key on the DOM while listening to route changes. However, I later discovered that getServerSideProps wasn't executing, and the returned data was still the old data.

https://github.com/user-attachments/assets/68f6af60-1f45-4aff-8b4b-dbfee3e10c3b

this is expected

@icyJoseph

icyJoseph commented 1 month ago

Right, that's strange, I could see the json requests when I tried this the first time... 🤔 let met try again...

icyJoseph commented 1 month ago

Is there any way you might be using shallow navigation, or somehow hijacking GSSP's normal behavior? I remember there's a hook out there to do some mutations to the page, that prevent GSSP from re-running. No Service Workers either?

rick-liruixin commented 1 month ago

We haven't made any special modifications. We are currently using middleware, but it doesn't seem related to this issue. This problem still occurs sporadically. You can test it on the links I provided.

import { NextRequest, NextResponse } from 'next/server'

const pagePermissions = ['/account']

export function middleware(request: NextRequest) {
  const nextPath = request.nextUrl.pathname
  if (
    !request.cookies.get('TOKEN')?.value.startsWith('customer') &&
    pagePermissions.some(path => nextPath.startsWith(path))
  ) {
    return NextResponse.redirect(new URL(`/login`, request.url))
  }

  return NextResponse.next()
}

export const config = {
  matcher: ['/account/:path*'],
}

Is there any way you might be using shallow navigation, or somehow hijacking GSSP's normal behavior? I remember there's a hook out there to do some mutations to the page, that prevent GSSP from re-running. No Service Workers either?

Can you elaborate on what you said above? @icyJoseph

icyJoseph commented 1 month ago

I swear, I still can't reproduce this... I always get the JSON calls. That middleware doesn't match these routes, so right now I wouldn't consider it.

Do you have to maybe, start at a specific page and navigate from there, for this to happen?

rick-liruixin commented 1 month ago

Okay, thank you. I will create a sample of the code used in my project tomorrow for your reference. @icyJoseph

rick-liruixin commented 1 month ago

I have one more question: does using shallow navigation cause getServerSideProps not to trigger? @icyJoseph

icyJoseph commented 1 month ago

I have one more question: does using shallow navigation cause getServerSideProps not to trigger? @icyJoseph

It does, but it is subtle... Consider this, https://nextjs.org/docs/pages/building-your-application/routing/linking-and-navigating#shallow-routing:

import { useEffect } from 'react'
import { useRouter } from 'next/router'

// Current URL is '/'
function Page() {
  const router = useRouter()

  useEffect(() => {
    // Always do navigations after the first render
    router.push('/?counter=10', undefined, { shallow: true })
  }, [])

  useEffect(() => {
    // The counter changed!
  }, [router.query.counter])
}

export default Page

Note that, we are at /, and we change only the query, with shallow true, and that skips remounting the page. However, if we change page (actual page file), then it doesn't matter if we do shallow navigation, the page unmounts.

Go to, https://stackblitz.com/edit/nextjs-bqzsj3?file=pages%2Fgssp%2F[slug].tsx and click the Go to example text

Now you see a timestamp, and a shallow and unshallow pair of links. Clicking the shallow link, doesn't change the timestamp, but clicking the unshallow link does...

rick-liruixin commented 1 month ago

Actually, there is a slight difference: I only use router.replace(url, undefined, { shallow: true }) when changing certain parameters in the address bar. But for page navigation, I don't use shallow: true.

Page: /category/[...url].tsx The general flow is:

  1. /categories/women/ready-to-wear.html
  2. Use router.replace(url, undefined, { shallow: true }) to navigate to /categories/women/ready-to-wear.html?page=2
  3. Use router.push('/categories/women.html') to navigate to /categories/women.html
  4. Use router.replace(url, undefined, { shallow: true }) to navigate to /categories/women.html?page=2

Then, when clicking the browser's forward and back buttons to switch between /categories/women/ready-to-wear.html?page=2 and /categories/women.html?page=2, the getServerSideProps sometimes doesn't trigger.

@icyJoseph

icyJoseph commented 1 month ago

I wonder if this is a bug on 13.5.6? or if shallow + middleware, is causing some odd behavior with some of your pages 🤔

When shallow routing is used with middleware it will not ensure the new page matches the current page like previously done without middleware. This is due to middleware being able to rewrite dynamically and can't be verified client-side without a data fetch which is skipped with shallow, so a shallow route change must always be treated as shallow.

But I am just guessing at this point, I can't reproduce the issue though (on your page) - how do you do step 3?

icyJoseph commented 1 month ago

Ah, I finally managed to reproduce the issue...

icyJoseph commented 1 month ago

This is just a shot in the dark, but, could you use the object URL form? https://nextjs.org/docs/pages/api-reference/functions/use-router#with-url-object

I suspect the url part is getting dropped in the history of the page. I don't want to write a long winded explanation, for something that might be wrong, but could you give it a go?

You'd need to specify the url as a query, as well as the page query param, and the pathname would be the actual file path.

rick-liruixin commented 1 month ago

Okay, I will replace router.push('/categories/women.html') with router.push({ pathname: '/category/[...url]', query: { url: ['women.html'] } }) as you suggested.

Example: Page: /category/[...url].tsx

  1. /categories/women/ready-to-wear.html
  2. Use router.replace(url, undefined, { shallow: true }) to navigate to /categories/women/ready-to-wear.html?page=2
  3. Use router.push({ pathname: '/category/[...url]', query: { url: ['women.html'] } }) to navigate to /categories/women.html
  4. Use router.replace(url, undefined, { shallow: true }) to navigate to /categories/women.html?page=2

Thank you. @icyJoseph

rick-liruixin commented 1 month ago

After testing with the above code flow, the issue still exists. @icyJoseph

icyJoseph commented 1 month ago

I meant to also change the replace calls, those are the problem I think... I also wonder if you need to pass an actual value, rather than undefined into the calls 😅 It has meaning...

rick-liruixin commented 1 month ago

In router.replace(url, undefined, { shallow: true }), the undefined parameter is essentially a placeholder indicating that we do not wish to pass a value for the second argument. We only want to utilize the { shallow: true } option. In our scenario, we want the product listing page to display the page number in the address bar, so we include the page number in the URL. This is the only use case we have for this feature.

@icyJoseph

icyJoseph commented 1 month ago

Sort of, not quite, I know what you are thinking of, and yes that's a valid pattern they allow, but that second parameter has meaning.

Image

In the Link component, that's the as prop, but in the router methods, it is the second parameter.

router.push(url, as, options) // same for replace

as: UrlObject | String - Optional decorator for the path that will be shown in the browser URL bar. Before Next.js 9.5.3 this was used for dynamic routes.

So, I'd like you to use the as value as well, which would be, what the browser URL should show, and the first argument, the file path /[...slug]. Make sure to get it working correctly, it has been a while since I used these.

The reason why I think something fishy is going on there, is the history state I see saved. Before page 2:

{
 as: "/categories/women/ready-to-wear.html",
 key: "0037cbki",
 options: {locale: undefined, _shouldResolveHref: true},
 url: "/categories/[...url]?url=women&url=ready-to-wear.html"
}

After:

{
 as: "/categories/women/ready-to-wear.html?page=2",
 key: "0037cbki",
 options: {scroll: false, shallow: true, _shouldResolveHref: true},
 url: "/categories/[...url]?page=2",
 __N: true
}
rick-liruixin commented 1 month ago

Got it, thank you for your feedback. I will replace the as parameter in router.push(url, as, options) with the actual value instead of undefined. Then, I will proceed with testing to see if this resolves the issue.

@icyJoseph

rick-liruixin commented 1 month ago

Hi, after testing your suggestions, I still encounter the same issue. Are there any other solutions?

Here is an example: https://github.com/rick-liruixin/next-getServerSideProps-bug-demo/blob/main/pages/categories/%5B...url%5D.jsx

If possible, could you test it on my demo? @icyJoseph

rick-liruixin commented 3 weeks ago

Hello, is there any scheduled time for this fix? Please inform me under this issue once the fix is applied in the new version. @icyJoseph

icyJoseph commented 3 weeks ago

Hi, sorry I just got back from a long weekend. I'll look into your repository now.

rick-liruixin commented 1 week ago

Hello, how's the progress going? @icyJoseph