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

next/link prefetch=false broken for dynamic routes #50670

Closed
1 task done
cjonesdoordash opened this issue Jun 2, 2023 · 6 comments · Fixed by #58413
Closed
1 task done

next/link prefetch=false broken for dynamic routes #50670

cjonesdoordash opened this issue Jun 2, 2023 · 6 comments · Fixed by #58413
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@cjonesdoordash
Copy link

cjonesdoordash commented Jun 2, 2023

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.4.0: Mon Mar  6 21:00:17 PST 2023; root:xnu-8796.101.5~3/RELEASE_X86_64
    Binaries:
      Node: 16.18.0
      npm: 8.19.2
      Yarn: 1.22.5
      pnpm: 6.23.6
    Relevant packages:
      next: 13.5.4
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.2.2

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

App directory (appDir: true), Routing (next/router, next/navigation, next/link)

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

https://github.com/cjonesdoordash/nextjs-prefetch-issue

To Reproduce

Run the example and click the link to navigate between pages. You'll notice that the async data fetched within the layout.tsx file that shouldn't be re-fetched between navigations is refetched every time (it will hit cache eventually, but this behavior is incorrect as layouts shouldn't be re-rendered)

Another interesting side effect of this is if you hard reload a page after you've hit the fetch cache is that you'll get new data, but clicking a link to navigate will actually give you a cache hit on the old cache entry, which definitely shouldn't happen.

Describe the Bug

To start off I've only been able to reproduce this issue when utilizing a dynamic segment (pages without a dynamic segment above them don't run into this issue).

If you use one and have pages below it that share a common layout and link to each other with prefetch set to false you can validate via console logs or fetch calls that the layout is re-ran on every navigation.

According to the docs layouts are meant to not re-render when you're just navigating within their segment.

Expected Behavior

Setting prefetch to false on links doesn't break the documented layout behavior for dynamic routes

Which browser are you using? (if relevant)

N/A

How are you deploying your application? (if relevant)

N/A

@cjonesdoordash cjonesdoordash added the bug Issue was opened via the bug report template. label Jun 2, 2023
@github-actions github-actions bot added area: app App directory (appDir: true) Navigation Related to Next.js linking (e.g., <Link>) and navigation. labels Jun 2, 2023
@cjonesdoordash
Copy link
Author

Validated this issue is still happening with the latest version 13.4.10 - any chance someone is able to take a peek at this? It breaks support with how i18n is recommended to be built.

@eposha
Copy link

eposha commented Sep 12, 2023

I have same issue

@cjonesdoordash

This comment has been minimized.

@cjonesdoordash
Copy link
Author

@timneutkens any chance you've been able to get someone to look into this issue?

timneutkens added a commit that referenced this issue Nov 16, 2023
### What?
When navigating between pages (via `prefetch: false`) within a dynamic
segment, the shared layout is re-rendered. This can cause unexpected
behavior like layout data changing when navigating between child
segments.

### Why?
When prefetch is false, we're currently opting into an "optimistic
navigation" codepath, which will optimistically render layout-routers up
to the point where data is missing, while kicking off data fetches. It
attempts to determine where refetching needs to happen by traversing the
router cache nodes and checking where data is missing. However, it
locates these cache nodes by using "segments" obtained by
[deconstructing the
URL](https://github.com/vercel/next.js/blob/fix/optimistic-bailout/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts#L142),
which won't accurately contain dynamic segment data. For ex, `/en` which
corresponds with `/app/[lang]/page.tsx` will have a cache node key of
`lang|en|d`, not `en`. Similarly, the optimistic tree that gets
constructed will also be incorrect, since it uses the URL segment.

### How?
My initial fix was to match the dynamic segment against the segment
constructed by the URL. But after discussion with @sebmarkbage and the
team, it seems more correct to remove the optimistic case all together
as there's no guarantee that the url will actually match to that
segment.

Fixes #50670

---------

Co-authored-by: Tim Neutkens <tim@timneutkens.nl>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@cjonesdoordash
Copy link
Author

Confirmed this issue is resolved with the latest canary. Thanks!

Copy link
Contributor

github-actions bot commented Dec 1, 2023

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 added the locked label Dec 1, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: app App directory (appDir: true) 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.

2 participants