RevalidateTag from Server Action changes default slot in parallel route #60815
Comments
1 task
|
Same issue here with 14.1.1-canary-48. I made a workaround by making |
|
Hi all -- this stems from a similar issue reported in #51714, which we have tracked in our backlog to investigate a solution for. I don't have an ETA on a fix currently, but it has been triaged and will be looked into. |
|
Thanks for the feedback guys. I am not entirely sure if my case has the same root cause as #51714 but seems like I am experiencing a similar issue as with @swa-legste. I have just posted a question on discussion (#62102) where I have found another behavior that seems to be linked to this issue's behavior. |
Closed
1 task
timneutkens
pushed a commit
that referenced
this issue
Mar 28, 2024
### What
When a parallel segment in the current router tree is no longer "active"
during a soft navigation (ie, no longer matches a page component on the
particular route), it remains on-screen until the page is refreshed, at
which point it would switch to rendering the `default.tsx` component.
However, when revalidating the router cache via `router.refresh`, or
when a server action finishes & refreshes the router cache, this would
trigger the "hard refresh" behavior. This would have the unintended
consequence of a 404 being triggered (which is the default behavior of
`default.tsx`) or inactive segments disappearing unexpectedly.
### Why
When the router cache is refreshed, it currently fetches new data for
the page by fetching from the current URL the user is on. This means
that the server will never respond with the data it needs if the segment
wasn't "activated" via the URL we're fetching from, as it came from
someplace else. Instead, the server will give us data for the
`default.tsx` component, which we don't want to render when doing a soft
refresh.
### How
This updates the `FlightRouterState` to encode information about the URL
that caused the segment to become active. That way, when some sort of
revalidation event takes place, we can both refresh the data for the
current URL (existing handling), and recursively refetch segment data
for anything that was still present in the tree but requires fetching
from a different URL. We patch this new data into the tree before
committing the final `CacheNode` to the router.
**Note**: I re-used the existing `refresh` and `url` arguments in
`FlightRouterState` to avoid introducing more options to this data
structure that is already a bit tricky to work with. Initially I was
going to re-use `"refetch"` as-is, which seemed to work ok, but I'm
worried about potential implications of this considering they have
different semantics. In an abundance of caution, I added a new marker
type ("`refresh`", alternative suggestions welcome).
This has some trade-offs: namely, if there are a lot of different
segments that are in this stale state that require data from different
URLs, the refresh is going to be blocked while we fetch all of these
segments. Having to do a separate round-trip for each of these segments
could be expensive. In an ideal world, we'd be able to enumerate the
segments we'd want to refetch and where they came from, so it could be
handled in a single round-trip. There are some ideas on how to improve
per-segment fetching which are out of scope of this PR. However, due to
the implicit contract that `middleware.ts` creates with URLs, we still
need to identify these resources by URLs.
Fixes #60815
Fixes #60950
Fixes #51711
Fixes #51714
Fixes #58715
Fixes #60948
Fixes #62213
Fixes #61341
Closes NEXT-1845
Closes NEXT-2030
ztanner
added a commit
that referenced
this issue
Mar 28, 2024
### What
When a parallel segment in the current router tree is no longer "active"
during a soft navigation (ie, no longer matches a page component on the
particular route), it remains on-screen until the page is refreshed, at
which point it would switch to rendering the `default.tsx` component.
However, when revalidating the router cache via `router.refresh`, or
when a server action finishes & refreshes the router cache, this would
trigger the "hard refresh" behavior. This would have the unintended
consequence of a 404 being triggered (which is the default behavior of
`default.tsx`) or inactive segments disappearing unexpectedly.
### Why
When the router cache is refreshed, it currently fetches new data for
the page by fetching from the current URL the user is on. This means
that the server will never respond with the data it needs if the segment
wasn't "activated" via the URL we're fetching from, as it came from
someplace else. Instead, the server will give us data for the
`default.tsx` component, which we don't want to render when doing a soft
refresh.
### How
This updates the `FlightRouterState` to encode information about the URL
that caused the segment to become active. That way, when some sort of
revalidation event takes place, we can both refresh the data for the
current URL (existing handling), and recursively refetch segment data
for anything that was still present in the tree but requires fetching
from a different URL. We patch this new data into the tree before
committing the final `CacheNode` to the router.
**Note**: I re-used the existing `refresh` and `url` arguments in
`FlightRouterState` to avoid introducing more options to this data
structure that is already a bit tricky to work with. Initially I was
going to re-use `"refetch"` as-is, which seemed to work ok, but I'm
worried about potential implications of this considering they have
different semantics. In an abundance of caution, I added a new marker
type ("`refresh`", alternative suggestions welcome).
This has some trade-offs: namely, if there are a lot of different
segments that are in this stale state that require data from different
URLs, the refresh is going to be blocked while we fetch all of these
segments. Having to do a separate round-trip for each of these segments
could be expensive. In an ideal world, we'd be able to enumerate the
segments we'd want to refetch and where they came from, so it could be
handled in a single round-trip. There are some ideas on how to improve
per-segment fetching which are out of scope of this PR. However, due to
the implicit contract that `middleware.ts` creates with URLs, we still
need to identify these resources by URLs.
Fixes #60815
Fixes #60950
Fixes #51711
Fixes #51714
Fixes #58715
Fixes #60948
Fixes #62213
Fixes #61341
Closes NEXT-1845
Closes NEXT-2030
ztanner
added a commit
that referenced
this issue
Mar 28, 2024
`applyRouterStatePatchToTree` had been refactored to support the case of not skipping the `__DEFAULT__` segment, so that `router.refresh` or revalidating in a server action wouldn't break the router. (More details in this #59585) This was a stop-gap and not an ideal solution, as this behavior means `router.refresh()` would effectively behave like reloading the page, where "stale" segments (ones that went from `__PAGE__` -> `__DEFAULT__`) would disappear. This PR reverts that handling. The next PR in this stack (#63608) adds handling to refresh "stale" segments as well. Note: We expect the test case that was added in #59585 to fail here, but it is re-enabled in the next PR in the stack. Note 2: #63608 was accidentally merged into this PR, despite being a separate entry in the stack. As such, I've copied the issues from that PR into this one so they can be linked. See the notes from that PR for the refresh fix details. Fixes #60815 Fixes #60950 Fixes #51711 Fixes #51714 Fixes #58715 Fixes #60948 Fixes #62213 Fixes #61341 Closes [NEXT-1845](https://linear.app/vercel/issue/NEXT-1845) Closes [NEXT-2030](https://linear.app/vercel/issue/NEXT-2030) <!-- Thanks for opening a PR! Your contribution is much appreciated. To make sure your PR is handled as smoothly as possible we request that you follow the checklist sections below. Choose the right checklist for the change(s) that you're making: ## For Contributors ### Improving Documentation - Run `pnpm prettier-fix` to fix formatting issues before opening the PR. - Read the Docs Contribution Guide to ensure your contribution follows the docs guidelines: https://nextjs.org/docs/community/contribution-guide ### Adding or Updating Examples - The "examples guidelines" are followed from our contributing doc https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md - Make sure the linting passes by running `pnpm build && pnpm lint`. See https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md ### Fixing a bug - Related issues linked using `fixes #number` - Tests added. See: https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs - Errors have a helpful link attached, see https://github.com/vercel/next.js/blob/canary/contributing.md ### Adding a feature - Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. (A discussion must be opened, see https://github.com/vercel/next.js/discussions/new?category=ideas) - Related issues/discussions are linked using `fixes #number` - e2e tests added (https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs) - Documentation added - Telemetry added. In case of a feature if it's used or not. - Errors have a helpful link attached, see https://github.com/vercel/next.js/blob/canary/contributing.md ## For Maintainers - Minimal description (aim for explaining to someone not on the team to understand the PR) - When linking to a Slack thread, you might want to share details of the conclusion - Link both the Linear (Fixes NEXT-xxx) and the GitHub issues - Add review comments if necessary to explain to the reviewer the logic behind a change ### What? ### Why? ### How? Closes NEXT- Fixes # --> Closes NEXT-2903
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Link to the code that reproduces this issue
https://github.com/swa-legste/next-reproduction
To Reproduce
Current vs. Expected behavior
expected
As we're updating the cart using a server action I'm expecting the modal to change and not the page:
actual
After executing the action the main slot is empty:
Verify canary release
Provide environment information
Operating System: Platform: linux Arch: x64 Version: #1 SMP PREEMPT_DYNAMIC Mon Jan 1 18:15:01 PST 2024 Binaries: Node: 20.11.0 npm: 10.2.4 Yarn: 1.22.21 pnpm: 8.14.0 Relevant Packages: next: 14.0.5-canary.65 eslint-config-next: 14.0.4 react: 18.2.0 react-dom: 18.2.0 typescript: 5.3.3 Next.js Config: output: N/AWhich area(s) are affected? (Select all that apply)
App Router
Routing (next/router, next/navigation, next/link)
Which stage(s) are affected? (Select all that apply)
next dev (local), next build (local), next start (local)
Additional context
I've reported a similar bug using
redirectinstead ofrevalidateTag: #60814This might be related to #54173 and partially fixed (at least redirects executes) by #59585.
Root cause being the
revalidateTag('cart')in the server action.Also tested against
14.1.1-canary.1The text was updated successfully, but these errors were encountered: