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

calling router.refresh in a parallel route breaks routing without any error messages #54723

Closed
1 task done
akomm opened this issue Aug 29, 2023 · 8 comments · Fixed by #59585
Closed
1 task done

calling router.refresh in a parallel route breaks routing without any error messages #54723

akomm opened this issue Aug 29, 2023 · 8 comments · Fixed by #59585
Labels
bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@akomm
Copy link

akomm commented Aug 29, 2023

Verify canary release

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

Provide environment information

Operating System:
      Platform: linux
      Arch: x64
      Version: #1 SMP PREEMPT_DYNAMIC Sun Aug  6 20:05:33 UTC 2023
    Binaries:
      Node: 16.17.0
      npm: 8.15.0
      Yarn: 1.22.19
      pnpm: 7.1.0
    Relevant Packages:
      next: 13.4.20-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) of Next.js are affected? (leave empty if unsure)

App Router, Routing (next/router, next/navigation, next/link)

Link to the code that reproduces this issue or a replay of the bug

https://codesandbox.io/p/sandbox/wild-browser-3yjcn4

To Reproduce

  • Open the sandbox.
  • Navigate using the links on the page.
  • goto /create
  • click the "break" button
  • try goto /

Describe the Bug

Routing stops working, both router and Link after router.refresh().

Expected Behavior

Routing should work.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

@akomm akomm added the bug Issue was opened via the bug report template. label Aug 29, 2023
@github-actions github-actions bot added the Navigation Related to Next.js linking (e.g., <Link>) and navigation. label Aug 29, 2023
@akomm
Copy link
Author

akomm commented Aug 30, 2023

update 1: Rechecked this with 13.4.20-canary.19: bug still present
update 2: Rechecked this with 13.4.20-canary.35: bug still present
update 3: Pulling out all new projects from nextjs. Last time I did such a decision was ~7 years ago with angular for the exact same reason: shipping non-experimental features in a broken state for the sake of "pushing the deadline". Entire sections of features are simply broken, be it middleware or parallel/intercepting routes. If it was experimental, there would be a clear communication. No need for this sort of things. Thanks you / gl

It appears the bug also happens, when you call router.refresh() outside the parallel route, but while one is either 1. active or 2. soft-active, means you navigated away from it softly, but the "page" is still mounted. The router breakage happens at the route tree where router.refresh was called all the way down, but seem to continue working upwards.

@tyler-dot-earth
Copy link

tyler-dot-earth commented Nov 8, 2023

Ran into this issue today on 13.5.6 (the codesandbox linked in OP is @ 13.4.20-canary.35).

My issue presents the same way as described / in the codesandbox:

  • I have a parallel route (a modal)
  • The modal renders a "use client"; component which calls router.refresh()
  • After calling router.refresh():
    • the parallel route data does not refresh in the browser (though I can see the refreshed data in my CLI output via console.log)
    • the router breaks entirely and i cannot navigate anywhere

I don't have a solution, but I do have a workaround: instead of router.refresh(), i call router.push(`?${new Date().valueOf()}`); to append a querystring which, effectively, refreshes the parallel route data as expected. Unintuitive, but it works.

I'd really like to see some acknowledgement and progress on this issue, as breaking the router when router.refresh() is used seems pretty serious.

EDIT: After more time with this workaround, i think it may only help sporadically. Lately (2023-12-06) I am playing with moving tRPC mutations to instead use server actions so that I can use revalidatePath('/', 'layout') to revalidate all data on the backend rather than router.refresh/push on the frontend.

@flsilva
Copy link

flsilva commented Nov 24, 2023

I recreated this issue with Next v14.0.4-canary.14 and I could only reproduce it on localhost, on Vercel it does work.

While it's great to have it working on Vercel, we need it working on localhost too as that's where we build and test apps most of the time.

Link to Vercel deployment (can't reproduce):

https://next-v14-router-refresh-issue-1.vercel.app/

Link to GitHub repo (can reproduce on localhost):

https://github.com/flsilva/next-v14-router-refresh-issue-1

@akomm
Copy link
Author

akomm commented Nov 27, 2023

@flsilva you need that not only on localhost, but anywhere where you want to host outside of vercel.
I'm not surprised it works on Vercel. Glad I've left next behind me.

@tyler-dot-earth
Copy link

I recreated this issue with Next v14.0.4-canary.14 and I could only reproduce it on localhost, on Vercel it does work.

My experience with this bug in my own app is the opposite — much less frequent on localhost, much more frequent on Vercel.

@akomm
Copy link
Author

akomm commented Dec 7, 2023

I recreated this issue with Next v14.0.4-canary.14 and I could only reproduce it on localhost, on Vercel it does work.

My experience with this bug in my own app is the opposite — much less frequent on localhost, much more frequent on Vercel.

If not exact version are pinned in package.json and no lockfile is used this sort of thing can happen when you have multiple platforms with different versions of next and hence get seemingly arbitrary positives and negatives because many params change.

@tyler-dot-earth
Copy link

I found several related issues.

This one is probably most notable:
#54173 (comment)

The link above is to my own reproduction in next@14.0.5-canary.2, but you'll also find multiple videos of this issue and other reproductions.

ztanner added a commit that referenced this issue Dec 15, 2023
…59585)

### What?
There are a bunch of different bugs caused by the same underlying issue,
but the common thread is that performing any sort of router cache update
(either through `router.refresh()`, `revalidatePath()`, or `redirect()`)
inside of a parallel route would break the router preventing subsequent
actions, and not resolve any pending state such as from `useFormState`.

### Why?
`applyPatch` is responsible for taking an update response from the
server and merging it into the client router cache. However, there's
specific bailout logic to skip over applying the patch to a
`__DEFAULT__` segment (which corresponds with a `default.tsx` page).
When the router detects a cache node that is expected to be rendered on
the page but contains no data, the router will trigger a lazy fetch to
retrieve the data that's expected to be there
([ref](https://github.com/vercel/next.js/blob/5adacb69126e0fd7dff7ebd45278c0dfd42f6116/packages/next/src/client/components/layout-router.tsx#L359-L370))
and then update the router cache once the data resolves
([ref](https://github.com/vercel/next.js/blob/5adacb69126e0fd7dff7ebd45278c0dfd42f6116/packages/next/src/client/components/layout-router.tsx#L399-L404)).

This is causing the router to get stuck in a loop: it'll fetch the data
for the cache node, send the data to the router reducer to merge it into
the existing cache nodes, skip merging that data in for `__DEFAULT__`
segments, and repeat.

### How?
We currently assign `__DEFAULT__` to have `notFound()` behavior when
there isn't a `default.tsx` component for a particular segment. This
makes it so that when loading a page that renders a slot without slot
content / a `default`, it 404s. But when performing a client-side
navigation, the intended behavior is different: we keep whatever was in
the `default` slots place, until the user refreshes the page, which
would then 404.

However, this logic is incorrect when triggering any of the above
mentioned cache node revalidation strategies: if we always skip applying
to the `__DEFAULT__` segment, slots will never properly handle reducer
actions that rely on making changes to their cache nodes.

This splits these different `applyPatch` functions: one that will apply
to the full tree, and another that'll apply to everything except the
default segments with the existing bailout condition.

Fixes #54173
Fixes #58772
Fixes #54723
Fixes #57665

Closes NEXT-1706
Closes NEXT-1815
Closes NEXT-1812
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 Dec 30, 2023
agustints pushed a commit to agustints/next.js that referenced this issue Jan 6, 2024
…ercel#59585)

### What?
There are a bunch of different bugs caused by the same underlying issue,
but the common thread is that performing any sort of router cache update
(either through `router.refresh()`, `revalidatePath()`, or `redirect()`)
inside of a parallel route would break the router preventing subsequent
actions, and not resolve any pending state such as from `useFormState`.

### Why?
`applyPatch` is responsible for taking an update response from the
server and merging it into the client router cache. However, there's
specific bailout logic to skip over applying the patch to a
`__DEFAULT__` segment (which corresponds with a `default.tsx` page).
When the router detects a cache node that is expected to be rendered on
the page but contains no data, the router will trigger a lazy fetch to
retrieve the data that's expected to be there
([ref](https://github.com/vercel/next.js/blob/5adacb69126e0fd7dff7ebd45278c0dfd42f6116/packages/next/src/client/components/layout-router.tsx#L359-L370))
and then update the router cache once the data resolves
([ref](https://github.com/vercel/next.js/blob/5adacb69126e0fd7dff7ebd45278c0dfd42f6116/packages/next/src/client/components/layout-router.tsx#L399-L404)).

This is causing the router to get stuck in a loop: it'll fetch the data
for the cache node, send the data to the router reducer to merge it into
the existing cache nodes, skip merging that data in for `__DEFAULT__`
segments, and repeat.

### How?
We currently assign `__DEFAULT__` to have `notFound()` behavior when
there isn't a `default.tsx` component for a particular segment. This
makes it so that when loading a page that renders a slot without slot
content / a `default`, it 404s. But when performing a client-side
navigation, the intended behavior is different: we keep whatever was in
the `default` slots place, until the user refreshes the page, which
would then 404.

However, this logic is incorrect when triggering any of the above
mentioned cache node revalidation strategies: if we always skip applying
to the `__DEFAULT__` segment, slots will never properly handle reducer
actions that rely on making changes to their cache nodes.

This splits these different `applyPatch` functions: one that will apply
to the full tree, and another that'll apply to everything except the
default segments with the existing bailout condition.

Fixes vercel#54173
Fixes vercel#58772
Fixes vercel#54723
Fixes vercel#57665

Closes NEXT-1706
Closes NEXT-1815
Closes NEXT-1812
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants