backport: style loading and navigation fixes #69458
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi there 👋 It looks like this PR introduces broken links to the docs, please take a moment to fix them before merging:
Thank you 🙏 |
lubieowoce
changed the title
lubieowoce
force-pushed
the
backport-14-3-0/parallel-stylesheets-and-navigations
branch
2 times, most recently
from
66f1f1d to
994cad2
Compare
Failing test suitesCommit: 6d33095
Expand output● Prerender › should on-demand revalidate for fallback: blocking with onlyGenerated if generated Read more about building and testing Next.js in contributing.md.
Expand output● middleware - development errors › when there is a compilation error after boot › logs the error correctly Read more about building and testing Next.js in contributing.md. |
|
hmmm. i can reliably reproduce the failure in (can't reproduce |
lubieowoce
force-pushed
the
backport-14-3-0/parallel-stylesheets-and-navigations
branch
from
d79b5fe to
0c34729
Compare
lubieowoce
commented
Aug 30, 2024
| // https://github.com/facebook/react/pull/28951 | ||
| // but the backport branch is still on older react | ||
| // waitForNEffects: 1, | ||
| waitForNEffects: isNextDev ? 2 : 1, |
There was a problem hiding this comment.
@ztanner i adjusted this because __NEXT_EXPERIMENTAL_PPR=true still gave me a 2 here. any idea why this'd change here? that's a bit suspicious
When the router encounters a `stale` cache entry, it clears the `rsc` data so that it can be fetched in render. All navigations (even just for hash fragments) flow through the navigation reducer, which has logic to discard any existing cache entries when the cache is stale. This bug has become more obvious after removing the default 30s cache, which would previously have masked it. This updates the existing handling that clears flight data to not do so if only the hash changes as there would be no server changes in this case. <!-- 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: - 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 - 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 - 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 - 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 - 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 Closes NEXT- Fixes # -->
This adds a new `layerAssets` property (containing styles and script tags) to `FlightDataPath`. Previously these were lumped in with the `head` node, but we intentionally only ever render a single `head`, to avoid duplicating metadata. This would mean `<AppRouter />` would only ever render imported stylesheets for a single page in a racey way. However, since Float handles hoisting and deduping these style tags, we're safe to render them for each segment. This PR introduces no change in behavior, aside from sending `layerAssets` down from the server and storing it in the client router cache. These nodes aren't rendered -- this is done in #66300.
This takes the `layerAssets` property from the previous PR and actually renders it, replacing the previous style handling. This ensures that when multiple page segments are rendered on screen, all of their associated CSS files are loaded. The existing `findHeadInCache` method only ever returns a single head node, which means it’d miss stylesheets. Fixes #59308 Fixes #63465
This removes the `lazyDataResolved` property on the CacheNode -- it was added to avoid repeatedly applying the server-patch action while data is missing (and while waiting for the server to respond), but if we just move the patch action be attached to then server fetch, there's no need to separate these (we're already suspending indefinitely while data is missing)
…67435) This PR was originally intended to ensure style/script tags were always rendered for all visible parallel routes, as before only the "first" child was rendered, similar to `head`. However, it caused a regression in PPR navigations, as the pending task on the `layerAssets` CacheNode meant that the `useDeferredValue` call in `layout-router` wouldn't be able to immediately switch to the prefetched RSC payload, as it was blocked by the dynamic response. Since `rsc` and `layerAssets` are both created by the same thing, this stack of PRs rewinds some of that work to combine them. This PR only reverts the previous implementation, and adds a test for the PPR navigation case that was failing. **Note: While I've split these PRs out for readability, this should land with #67436 as this PR by itself does not restore the CSS behavior that was fixed in the original implementation.**
During SSR, we walk the component tree and pass and immediately seed the router cache with the tree. We also seed the prefetch cache for the initial route with the same tree data because navigations events will apply the data from the prefetch cache. The rationale for this is that since we already have the data, we might as well seed it in the prefetch cache, to save a server round trip later. The way we're seeding the cache introduced a bug when PPR is turned on: PPR expects that each element in `FlightDataPath` is of length 3, corresponding with: `[PrefetchedTree, SeedData, Head]`. However, we were seeding it with 4 items: `[Segment, PrefetchedTree, SeedData, Head]`. When the forked router implementation sees anything other than 3 items, it reverts back to the pre-PPR behavior, which means the data will be lazy-fetched during render rather than eagerly in the reducer. As a result, when navigating back to the page that was seeded during SSR, it wouldn't immediately apply the static parts to the UI: it'd block until the dynamic render finished. This PR also includes a small change to ensure reused tasks don’t trigger a dynamic request. A navigation test was failing because this PR fixed a bug that was causing it to hit the non-PPR behavior, which already handles this correctly. I've added an explicit test for this in PPR. I've also excluded all of the `client-cache` tests from PPR, as many of them were already failing and aren't relevant to PPR, since PPR doesn't currently make use of the `staleTime`/ cache heuristics that the regular router does.
lubieowoce
force-pushed
the
backport-14-3-0/parallel-stylesheets-and-navigations
branch
from
0c34729 to
6d33095
Compare
|
Isolated these changes to just the relevant fix in: |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
mutable.onlyHashChangerequired by another PR)layerAssetsproperty from FlightData #67435