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.

Sign up for GitHub

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

Jump to bottom

Server Actions Inside Slots Do not work as expected #57665

Closed
1 task done
joelrozen opened this issue Oct 28, 2023 · 1 comment · Fixed by #59585
Closed
1 task done

Server Actions Inside Slots Do not work as expected #57665

joelrozen opened this issue Oct 28, 2023 · 1 comment · Fixed by #59585
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@joelrozen
Copy link

joelrozen commented Oct 28, 2023 •
edited by padmaia
Loading

Link to the code that reproduces this issue

https://github.com/joelrozen/nextgram

To Reproduce

  1. Install dependencies and start application (npm i && npm run dev)
  2. Click a photo so it opens in the Modal
  3. Click "Redirect Me" Button
  4. Observe in server console "action triggered" gets logged, however the page never redirects back to '/'. Inspecting the form element of the modal also reveals that the form action is javascript:throw new Error('A React form was unexpectedly submitted... and none of the hidden nextjs form inputs are injected.
  5. Hard refresh the photo route so that the image loads in the page instead of the Modal slot.
  6. Observe HTML of form looks correct, click the "Redirect Me" button
  7. Observer "action triggered" gets logged to server console and 1 second later, the page is redirected back to "/"

Current vs. Expected behavior

Expect server actions inside forms to work consistently no matter where they're rendered (page, slots, intercepted/parallel routes etc).

Verify canary release

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

Provide environment information

Operating System:
  Platform: darwin
  Arch: x64
  Version: Darwin Kernel Version 22.1.0: Sun Oct  9 20:14:54 PDT 2022; root:xnu-8792.41.9~2/RELEASE_X86_64
Binaries:
  Node: 18.18.1
  npm: 9.8.1
  Yarn: 1.22.19
  pnpm: 7.13.4
Relevant Packages:
  next: 14.0.1-canary.1
  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)

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

Additional context

same behaviour observed when using useFormState

NEXT-1812
@joelrozen joelrozen added the bug Issue was opened via the bug report template. label Oct 28, 2023
@github-actions github-actions bot added the Navigation Related to Next.js linking (e.g., <Link>) and navigation. label Oct 28, 2023
@pekarja5 pekarja5 mentioned this issue Nov 20, 2023
Closed
1 task
@chrisb2244 chrisb2244 mentioned this issue Nov 24, 2023
Closed
1 task
@balazsorban44 balazsorban44 added the linear: next Confirmed issue that is tracked by the Next.js team. label Dec 6, 2023
@ztanner ztanner mentioned this issue Dec 14, 2023
Merged
ztanner added a commit that referenced this issue Dec 15, 2023
@ztanner
Fix parallel routes with server actions / revalidating router cache (#…
dd57054 
…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
@github-actions GitHub Actions
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
@ztanner @agustints
Fix parallel routes with server actions / revalidating router cache (v…
e6e008c 
…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.
Assignees
No one assigned
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix parallel routes with server actions / revalidating router cache vercel/next.js
2 participants
@balazsorban44 @joelrozen