vercel / next.js

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

[NEXT-1141] Next router / link not working when redirecting with new search params [Regression] #49297

Open mikebuilds opened 1 year ago

mikebuilds commented 1 year ago

Verify canary release

Provide environment information

Operating System:
      Platform: darwin
      Arch: arm64
      Version: Darwin Kernel Version 22.1.0: Sun Oct  9 20:19:12 PDT 2022; root:xnu-8792.41.9~3/RELEASE_ARM64_T6020
    Binaries:
      Node: 18.15.0
      npm: 9.5.0
      Yarn: N/A
      pnpm: 8.0.0
    Relevant packages:
      next: 13.4.0
      eslint-config-next: 13.3.0
      react: 18.2.0
      react-dom: 18.2.0

Which area(s) of Next.js are affected? (leave empty if unsure)

No response

Link to the code that reproduces this issue

https://codesandbox.io/p/sandbox/eloquent-https-5slx4d

To Reproduce

Navigate to the same URL with a new search param either via router.push() or the link component.

In the reproduction, wait for the initial page to load - then try and change to page 2, or 3, or use the search input.

Describe the Bug

The bug first appeared in 13.3.5-canary.2, prior versions works correctly.

Navigating to the same page with new search params no longer renders the loading.tsx. Looking at the URL / network tab - it seems that the router does kick off the fetch for the new page, but then waits for the full body response before updating the URL, or rendering the loading page.

Expected Behavior

Prior to 13.3.5-canary.2, the behaviour was working correctly:

Navigating to the same page with new search params should show the loading.tsx, if the page is slow to fetch.

Video of this working previously:

https://user-images.githubusercontent.com/3614846/236453767-429cec22-46bc-436f-8377-2a597243a866.mov

Which browser are you using? (if relevant)

N/A

How are you deploying your application? (if relevant)

N/A

NEXT-1141

mikebuilds commented 1 year ago

For reference, I think this may be the issue commit: https://github.com/vercel/next.js/commit/25ba8a74b7544dfb6b30d1b67c47b9cb5360cb4e @feedthejim

Unsure if there is background here, but I can't think of why this could potentially be desired behaviour for any component using search params.

feedthejim commented 1 year ago

@mikebuilds can you perhaps share the code for the issue 2? It's hard to investigate because it could be a mistake in your code.

Looking into issue 1

mikebuilds commented 1 year ago

@feedthejim apologies, issue 2 is in fact a code issue - I changed some code when trying to debug issue 1, and accidentally introduced duplicate keys on the table rows.

Appreciate you looking into issue 1

fprl commented 1 year ago

Hello there,

Issue 1 is happening to me + other issue where next fetch don't take into account the cache and revalidate options in the fetch call and its only running the fetch again after 30s approx when navigating back to the route:

{
      cache: 'no-store',
      next: {
        revalidate: 0,
      },
}

PS: Found my problem and its probably related: https://github.com/vercel/next.js/issues/49300

fprl commented 1 year ago

Hey everyone, is there any update on this? loading.tsx is not rendering once searchParams has changed after using the example in the docs.

psikoi commented 1 year ago

What I've done to circumvent this issue is to use my own suspense boundary (with a key prop) instead of relying on the loading.tsx file's boundary.

In the Suspense's key prop I stringify the search params, that way it'll re-render (and fallback) whenever the search params change.

With this approach I was also able to exclude certain params from influencing the loading state, like the currently opened dialog.

example: https://github.com/wise-old-man/wise-old-man/blob/53bbe450f845d220895a46eee3f9da903f34674f/app-v2/src/app/names/page.tsx#L55

fprl commented 1 year ago

What I've done to circumvent this issue is to use my own suspense boundary (with a key prop) instead of relying on the loading.tsx file's boundary.

In the Suspense's key prop I stringify the search params, that way it'll re-render (and fallback) whenever the search params change.

With this approach I was also able to exclude certain params from influencing the loading state, like the currently opened dialog.

example: https://github.com/wise-old-man/wise-old-man/blob/53bbe450f845d220895a46eee3f9da903f34674f/app-v2/src/app/names/page.tsx#L55

I tried this, and it's only working for me in dev but not in prod:

import { Suspense } from 'react';

import { getHouses } from '@/utils/houses';

import Listings from '../components/listings';

type Props = {
  searchParams: {
    [key: string]: string;
  };
};

export default async function SearchPage({ searchParams }: Props) {
  const key = JSON.stringify(searchParams);
  const listingsPromise = getHouses(searchParams);

  return (
    <Suspense key={key} fallback={<Listings.Skeleton />}>
      {/* @ts-expect-error Async Server Component */}
      <Listings listingsPromise={listingsPromise} />
    </Suspense>
  );
}

PS: Keep in mind I'm sending the promise and not the result so that shouldn't be a problem.

chillardinho commented 1 year ago

@francoromanol have you tried to deploy it to somewhere other than vercel? I'm not sure but might be related to https://github.com/vercel/next.js/issues/51033.

there's also a similar problem mentioned in https://github.com/vercel/next.js/issues/42346 😭

tantsur commented 1 year ago

Guys, any news on that?

alepacheco commented 1 year ago

I had this issue today and had a difficult time finding a solution until i found this thread, using a key on the Suspense Element fixes it.

Let's get this issue resolved 🙏

MelancholYA commented 1 year ago

i added the JSON.stringify(searchParams) as a key on the suspense and it worked not in production tho , it only works in dev env

jaredgalanis commented 1 year ago

Any updates on this?

Even with a key on Suspense I'm not seeing the loading template when updating query params and pushing into the router.

jaredgalanis commented 1 year ago

Any updates on this?

Even with a key on Suspense I'm not seeing the loading template when updating query params and pushing into the router.

I've actually realized that while this may not be expected behavior, I don't want to reload the entire fallback on query param changes. I'd rather use useTransition to provide a more subtle loading indicator per the React docs https://react.dev/reference/react/Suspense#preventing-unwanted-fallbacks.

psikoi commented 1 year ago

Any updates on this? Even with a key on Suspense I'm not seeing the loading template when updating query params and pushing into the router.

I've actually realized that while this may not be expected behavior, I don't want to reload the entire fallback on query param changes. I'd rather use useTransition to provide a more subtle loading indicator per the React docs https://react.dev/reference/react/Suspense#preventing-unwanted-fallbacks.

Yes, this is something I have come to realize too, I now see the loading skeleton as the state for "no data yet", but if I'm updating the UI with new data (maybe due to some search or filters being changed in the URL), I prefer the user to still be able to see the previous results, with some more inline and subtle indication that the data is changing.

Here's an example in my app:

https://wiseoldman.net/competitions

You'll see that as you first visit the page, there's a loading skeleton, but if you type in any search or select a filter, there's a subtle inline loading indicator that uses useTransition.

I found myself doing weird things like adding a key prop to the Suspense boundary, it was usually an indication that I was fighting the mental model of the app router because I thought that all loading states are equal. I do not believe they are, anymore.

tonimercadante commented 1 year ago

I'm doing a filter based on searchParams and I need to trigger the reload when the params change. Looking in the comments here it seems that in previous versions of Next it worked, I'm using version 14.0.3 and changing the params do not trigger the loading.tsx page.

peiris commented 11 months ago

I'm doing a filter based on searchParams and I need to trigger the reload when the params change. Looking in the comments here it seems that in previous versions of Next it worked, I'm using version 14.0.3 and changing the params do not trigger the loading.tsx page.

Same problem here

nghyane commented 11 months ago

I'm doing a filter based on searchParams and I need to trigger the reload when the params change. Looking in the comments here it seems that in previous versions of Next it worked, I'm using version 14.0.3 and changing the params do not trigger the loading.tsx page.

Same problem here :((

judgemavi commented 11 months ago

I experimented a little and found that using a regular anchor tag "a" instead of Link trigger loading.tsx every time.

I guess all assets are reloaded when using tag "a"

SkyGopnik commented 10 months ago

+1

Edit by maintainers: Comment was automatically minimized because it was considered unhelpful. (If you think this was by mistake, let us know). Please only comment if it adds context to the issue. If you want to express that you have the same problem, use the upvote 👍 on the issue description or subscribe to the issue for updates. Thanks!

MrLoh commented 10 months ago

@psikoi is right that this is not a bug, but that the right solution is to use useTransition to show a more subtle localized loading state while query params are changing.

abaksha-sc commented 9 months ago

Hm, I even tried to generate page URLs like /report/page/X and this doesn't help. So seems like issue is not only with search params

ymonye commented 8 months ago

{ searchParams }: Props

Did you ever get this resolved? I'm also on Next 14 with the same issues

LuizFernando991 commented 7 months ago

https://www.youtube.com/watch?v=UNEXbGJCTw8

In this video he demonstrates several techniques, the best for me is the one in minute 17

adpadillar commented 4 months ago

Just in case this seems useful to anyone. My usecase included doing router.push calls to add navigate to the same page but with new search params. I wanted to trigger a loading indicator while the server replaced the old data with the new data. I tried managing my own suspense boundary with the key prop like @psikoi suggested, but I needed more granular control.

After reading @MrLoh 's comment, I tried to follow his suggestion. My solution ended being something like this

Instead of doing:

router.push(urlWithNewParams)

I did the following:

import { useTransition } from "react"

// Inside the component

const [isPending, startTransition] = useTransition()

const navigate = useCallback((newPath: string) => {
    startTransition(() => {
        router.push(newPath)
    })
})

Then, wherever I was calling router.push, I replaced with navigate. Finally, I was able to use the isPending value to display a loading spinner in the JSX

return (
    <>
        {isPending && <Loader2 className="animate-spin" />
    </>
)

This allowed me to show some indication to the users that there is more data coming in, without removing the old data from the page. Hope this helps someone!

haraldev01 commented 3 months ago

I've been toiling with this issue for the past days trying to find a solution that works. First about the video that @LuizFernando991 linked; I think that only works when you're manually using suspense boundaries within a page component. It doesn't seem to work with the suspense boundary between a layout and a page component. I was able to get it to work a little bit, but this was by dynamically rendering the entire layout, and this was also unreliable.

I'm pretty sure if my understanding of Next.Js is correct, the reason the old page doesn't get removed right away is because of useTransition. According to the react docs: "Suspense-enabled routers are expected to wrap the navigation updates into Transitions by default."

from: https://react.dev/reference/react/Suspense#resetting-suspense-boundaries-on-navigation

That part of that page is very relevant for the issue we are facing, and indeed the intended solution is to use the searchParams as a key for the component to tell React that although the path is the same, the content is different, and so it should fall back to the suspense boundary. The fact that this doesn't seem to work in Next is pretty disappointing.

manacy-keyvalue commented 3 months ago

Facing the same issue. At present, fixing it as described in https://github.com/vercel/next.js/issues/49297#issuecomment-1568557317. Hoping to see a fix soon.