[Bug] next/link is not calling the server for subsequent navigations on dynamic routes

Fredkiss3 commented 2 years ago

Describe the feature you'd like to request

In the documentation it is said that the conditions for hard navigation are :

when navigating between dynamic segments
When navigating between two different group layouts (ex: from (groupA)/layout to (groupB)/layout )

I'd like to suggest also adding hard navigation for segments marked with dynamic='force-dynamic' or when using dynamic functions and even when using fetch with cache: 'no-store'.

In the docs you said that using these configurations is like using getServerSideProps() in the pages directory, but it does not behave the same between navigations which is quite confusing.

Use cases for this feature could be these :

this app that only store pokemon data in cookies (Live here and source here)
this simple case for an app that generate a random number each time : ckblitz.com/edit/nextjs-sxijav?file=package.json,app%2Fnested%2Fpage.tsx

Describe the solution you'd like

The solution i propose is to consider hard navigation for these cases :

When navigating to a page marked with dynamic='force-dynamic', next should always do a hard navigation
When navigating to a page using dynamic functions headers() and cookies(), next should always do a hard navigation
When navigating to a page using fetch with cache: 'no-store', next should always do a hard navigation, or at least next should always refetch the data
When navigating to a page using either fetch with next: { revalidate: n_seconds } or export const revalidate = n_seconds, next should only do hard navigation when the n_seconds has elapsed.

The last two could be tricky and if it is not ideal, at least add a paragraph in the doc explaining why it is not possible and maybe recommending the first two approaches.

Describe alternatives you've considered

Updates

In next@13.3.1, using a <Link prefetch={false} /> seems to fix the problem : https://stackblitz.com/edit/vercel-next-js-n1tqpr?file=app%2Flayout.tsx . But it disables link prefetching and result in slower navigations.
As of next@13.4.0 the client side cache has been reworked, dynamic pages are now cached in the client with a timer of 30 seconds, so every 30 seconds your server is recalled, with one catch : this only apply if you navigates to a different page after that time, if you do very fast back & forth to the same page, the timer will reset and only wait for another 30 seconds. Disabling prefetch does not solve the problem anymore, it disables prefetching, but once the page has been navigated to, the rules of 30 seconds still applies. If you want the old behaviour, you'd have to downgrade to next@13.3.1 for now, You will loose support for Server Actions & revalidate primitives though, Or you can also not use the Link component and use regular a tags if you still want the new features of Next.

However there are things in the work for both allowing prefetching on hover (with <Link>) and allowing for setting the stale time on the client side cache. You can read in the PR about the client side rework :

Follow ups

we may add another API to control the cache TTL at the page level

a way to opt-in for prefetch on hover even with prefetch={false}

This would allow for setting a pageTTL = 0 for calling the server on each & every navigation to always have fresh data.

_NEXT-1352

Fredkiss3 commented 1 year ago

Any news yet about this feature ?

zenflow commented 1 year ago

I believe this proposal is to fix these issues:

42546
43879

I am strongly in favor changing/fixing the rules of when soft vs hard navigation happens, to support dynamic data on pages that have no dynamic segments. You can see from the docs that soft navigation (i.e. client side cache) will always be used in this case (https://beta.nextjs.org/docs/routing/linking-and-navigating#conditions-for-soft-navigation) and there's no way to override this. There's effectively no way to bust the client side cache for these pages except a manual router.refresh().

At the same time, I'm against the exact proposal. I'd prefer to keep rules for client-side caching (hard vs soft navigation) separate/independent from rules for server-side caching (dynamic='force-dynamic', cache: 'no-store', etc). There seems to be some confusion around the fact that we are dealing with two separate caches and the proposed solution would definitely add to that, as well as make it more difficult learn and reason about how next.js is supposed to behave in whatever situation.

@leerob Can we have an option like export const navigation = 'hard' to force hard navigation on pages where it's needed?

Fredkiss3 commented 1 year ago

For sure, conceptually there are two caches, but to the developer who doesn’t understand NextJs RSC that deep it does not seem like it. I’ve read the docs more than 3 or 4 times, yet I didn’t see any mention of the two caches as separate, there is dynamic rendering, dynamic segments and dynamic functions which are very confusing. And for the longest time I thought they were all equivalent or similar.

As for adding a new option, this would confuse people more I think. Maybe it is the best for performance, but DX wise it is not ideal. And more, it would be difficult to wrap our head around how it would play with the rest of options (dynamic rendering, dynamic segments & functions).

But IMO, when NextJs says in their doc that export const dynamic="force-dynamic" behaves like gSSP in pages directory I expect it to behave like it and rerender on page navigation, if not then the docs should precise that caveat and give us a way to make it behave exactly like gSSP.

zenflow commented 1 year ago

True that adding a new option might add some confusion in the sense that it's more cognitive overhead (more to learn from docs and think about for every page) and I think we definitely already have enough of that with app/ dir, so I definitely want to keep it as simple as possible. I have given it some more thought..

So the Conditions for Soft Navigation (new in app/ dir) is causing our problem, inaccurately trying to guess whether hard or soft nav is appropriate:

On navigation, Next.js will use soft navigation if the route you are navigating to has been prefetched, and either doesn't include dynamic segments or has the same dynamic parameters as the current route.

For example, ...

Why do these conditions even involve whether the page has dynamic segments or not? Does it make sense?

(our problem) For pages without dynamic segments, we may or may not want to skip frontend cache, since these pages might also have dynamic data which we want/need to be fresh.
(also) For pages with dynamic segments, we may or may not want to have frontend cache, since these pages might also have data which we don't need to be so fresh, and we can take advantage of client side cache to reduce requests to server.

The ideal solution IMO:

soft vs hard navigation applied to pages regardless of dynamic segments or their params
static pages always use soft navigation (already the case i looks like, judging by docs)
dynamic pages default to hard navigation & can opt in to using soft navigation (i.e. client side cache) via export const navigation = 'soft' or some option like that

What do you think? There would be a new option, but you wouldn't have to use it or worry about it unless you wanted to optimize performance & reduce requests to server for a dynamic route, with using a feature that's new in app/ dir.

Fredkiss3 commented 1 year ago

I’m all in on your proposition 👍

njarraud commented 1 year ago

The current state with hard/soft navigation as well as prefetch is still very buggy and I would love seeing the improvements stated above.

prefetch={false} currently fully reloads the page (with loss of state that goes with). The SSR components also run twice (navigate from homepage to page1 in example)
In the documentation it is stated that soft navigation occurs if the route you are navigating to has been prefetched, and either doesn't include dynamic segments or has the same dynamic parameters as the current route. It is actually not fully true. Navigating from page1 or page2 to page1/123will actually perform a hard navigation but from / to page1/123 does not (soft navigation).

Link to my simple example https://codesandbox.io/p/sandbox/nextjsappdirnav-d6clzo I console log on the server when the SSR components are rendered.

page 1 links are prefetch={false}

Currently I cannot find a solution to force re-rendering SSR pages (hard navigation) as prefetch false is broken.

Fredkiss3 commented 1 year ago

@njarraud I don't remember where i've seen this but someone suggested to create a client component solely for the sake of refreshing the page, something like this :

// components/refresh-page
'use client';

import { useRouter } from 'next/navigation';

export function RefreshPage() {
  const router = useRouter();
  useEffect(() => {
      router.refresh();
  }, []);
  return null;
}

// page.tsx

export default function Page() {
  return (
   <>
      <RefreshPage />
       {/*... rest of your server-component code*/}
   </>
 )
}

But to be honest with you this workaround is not ideal.

njarraud commented 1 year ago

Thanks. It kinda does the trick-ish as a quick and dirty temporary "fix". It renders the page twice on first page load (production) and multiple times in dev. Just have to cross my fingers for a proper fix from the next team in the future.

hubertlepicki commented 1 year ago

@Fredkiss3 thanks for the code, you saved the day! But indeed, a solution like @zenflow suggests would be ideal.

To add to tiscussion, I think this is only a problem if a link leading to such dynamic page is prefetching, which is unfortunately the default. The other workaround I found was to disable link prefetching, i.e. if I do:

<Link to="/random" prefetch={false}>Click me</Link>

the above does seem to not use the cache as my random page generation page returns a different number every time, but I also think the whole page reloads on link click as a result so that's suboptimal. I think this may be a separate bug, as the Link now behaves like a tag with no extra behavior added.

Another workaround I found is not to use the Link at all, but instead useRouter and do push(href) directly. I wrapped it in a simple component and have started using instead of Link when linking to dynamic pages, seems to work just fine without doing any caching or any prefetching.

'use client';

import { useRouter } from 'next/navigation';

export default function DynamicLink({href, children}) {
  const router = useRouter();

  return(
    <a href={href} onClick={(e) => {e.preventDefault(); router.push(href)}}>{children}</a>
  );
}

I think this is the best solution I found so far.

Fredkiss3 commented 1 year ago

I think @hubertlepicki your solution is just better 🤩, and i think i will use that for now until this issue is fixed. To add to this, i modified the code to be more complete with typescript :

'use client';

import { useRouter } from 'next/navigation';
import { forwardRef } from 'react';

const DynamicLink = forwardRef<
  HTMLAnchorElement,
  Omit<React.HTMLProps<HTMLAnchorElement>, 'ref'>
>(({ href, children, ...props }, ref) => {
  const router = useRouter();

  return (
    <a
      {...props}
      ref={ref}
      href={href}
      onClick={(e) => {
        e.preventDefault();
        router.push(href);
      }}
    >
      {children}
    </a>
  );
});

export default DynamicLink;

hubertlepicki commented 1 year ago

@Fredkiss3 ah yes and passing the props to the a tag, indeed that makes sense!

Fredkiss3 commented 1 year ago

Was experimenting with this behavior and it seems that you can force next to always do a hard navigation if you pass a querystring to the link component.

Updated the example by just adding a querystring, and it seems to work : https://stackblitz.com/edit/nextjs-smotka?file=app%2Fpage.tsx,app%2Fnested%2Fpage.tsx

This have the advantage of the page being prefetched on the first render, and always refetched on every navigation.

One other thing is noticed is that without this hack, when the page has been prefetched, it will load on the first navigation and next will scroll to the top of the page, but on subsequent navigations it doesn't. When you introduce this hack, it will always scroll to the top [video example down here].

https://user-images.githubusercontent.com/38298743/211924001-51f2ec85-0647-4b76-a924-f6f28de1d90e.mov

Warning THIS IS DEFINITELY A BUG

Josehower commented 1 year ago

This seems to be a real issue that need to be considered by Next.js Team. It would have a lot of implications for components trying to use cookies authentication before rendering and is definitely going to make very difficult to migrate some apps relying on gSSP.

As some people has mentioned before the docs are not accurate mentioning using 'force-dynamic' is equivalent to getServerSideProps()

https://beta.nextjs.org/docs/api-reference/segment-config#dynamic:~:text=%27force%2Ddynamic%27%3A%20Force,fetchCache%20%3D%20%27force%2Dno%2Dstore%27

All what we have until now are some workarounds:

Create a component that force the router.refresh() on every render
- https://github.com/vercel/next.js/issues/42991#issuecomment-1363436609
create a useEffect() that calls router.refresh() on every render
Use <a> instead of <Link> to force Hard navigation
- https://github.com/vercel/next.js/issues/42991#issuecomment-1365230317
Use <Link prefetch={false}> to force Hard navigation
- https://github.com/vercel/next.js/issues/42991#issuecomment-1363429149
Add a query param to the link in order to force Hard navigation
- https://github.com/vercel/next.js/issues/42991#issuecomment-1379506711
Using fake dynamic segments to force Hard navigation
- https://stackblitz.com/edit/nextjs-51tdbm?file=app%2F[any]%2Fpage.tsx

On my opinion none of this solutions is good enough they are not as clean as should be. I agree with the people that think Next.js team should update the conditions for Soft Navigation or adding some kind of configuration to the server/client component make the server cache to take preference over the client cache.

Maybe consider any server component with export const dynamic = 'force-dynamic' or export const revalidate = 0; to use Hard navigation only.

I have being enjoying app directory a lot but. i'll keep checking since i really need this feature to be updated.

logemann commented 1 year ago

Was experimenting with this behavior and it seems that you can force the next to always do a hard navigation if you pass a querystring to the link component.

Uhhh. Thats a nice workaround. And so easy ;-)

But to comment on this overall topic. I think this is a major issue for NextJS. The expectation is totally different when reading the docs compared to the actual behavior. Even without reading the docs, i expect a force-dynamic component / revalidation component to re-fetch data (what it would do, if next/navigation wouldnt cache it if i am correct here).

Nobody will get the difference that a Browser Refresh does in fact work as expected wrt revalidation but a user navigation wont. I am strongly with @Josehower on the statement: "Maybe consider any server component with export const dynamic = 'force-dynamic' or export const revalidate = 0; to use Hard navigation only."

Fredkiss3 commented 1 year ago

Was experimenting with this behavior and it seems that you can force the next to always do a hard navigation if you pass a querystring to the link component.

After doing some tests again, it seems that this workaround only work if you are navigating from one segment to another different segment, navigation between the same segment will cache the result for the page with the querystring.

To illustrate : So navigating from /post/1?random to /post/2?random (they are the same segment since it is /posts/[id]) and vice versa, will only do hard navigation for the first time. If you do /search?random to /search?random2, next will fetch when the page for ?random, the do the same for ?random2 then when navigating back from one url to another next will not refetch the pages.

One way to always assure a hard navigation then is to generate a random querystring for the link, so that next will ignore the cache... The problem with this is that you could end up with hydration errors.

If is the case then, you could either : Add prop to tell react to ignore the error with suppressHydrationWarning :

const MyLink({href}) {
  return <Link suppressHydrationWarning href={`${href}?random=${Math.random()}`}>...</Link>;
}

Or use react useId to have an consistent id between server and hydration :

const MyLink({href}) {
  const id = React.useId();
  return <Link suppressHydrationWarning href={`${href}?random=${id}`}>...</Link>;
}

But then again you return to step zero where the id will be random per link and per page, but it will not change between renders so next will cache the page.

If i could understand nextjs code, i would try to fix this by myself as it is really bugging me, but i can't, so what i can do right now is giving workaround 😅.

njarraud commented 1 year ago

Since 13.1.2, navigation with url and query params is completely broken for me in production mode (not dev). The very first SSR page /discussions/1?page=1 will load correctly but navigating to /discussions/1?page=2 will reuse the browser cache from /discussions/1?page=1. It looks like the query params are completely ignored on further pages with the same segments but different query params. Even If I hard reload the page /discussions/1?page=2, it is still the browser cache for /discussions/1?page=1 which is displayed and no calls to the SSR server are made.

I downgraded back to 13.1.1.

karlhorky commented 1 year ago

@njarraud hm, this seems like a significant bug, maybe it's worth reporting this separately as a new issue, with a link back to this issue...

njarraud commented 1 year ago

Yes, I plan to make a minimal example, first to verify if nothing in my code is triggering this behaviour and secondly to report a bug properly if this is the case. Just need to find a bit of free time now.

[Edit] I have created a bug report for this issue that might not be related but pretty critical I believe #45026

focux commented 1 year ago

I'm on the latest Next.js version (v13.1.3) and the workaround using <a> and router.push isn't working.

Josehower commented 1 year ago

I'm on the latest Next.js version (v13.1.3) and the workaround using <a> and router.push isn't working.

right router.push() is soft navigation too, you may need to use

router.replace("/new-page");
router.refresh()

Josehower commented 1 year ago

As a more elaborated work around that basically avoid the creation or weird patterns on the code i just patched Next.js to disable soft navigation completely:

https://github.com/upleveled/next-13-app-dir-demo-project/blob/main/patches/next%2B13.1.4.patch

diff --git a/node_modules/next/dist/client/components/reducer.js b/node_modules/next/dist/client/components/reducer.js
index 951f016..947ce1f 100644
--- a/node_modules/next/dist/client/components/reducer.js
+++ b/node_modules/next/dist/client/components/reducer.js
@@ -317,6 +317,9 @@ function fillLazyItemsTillLeafWithHead(newCache, existingCache, routerState, hea
     return tree;
 }
 function shouldHardNavigate(flightSegmentPath, flightRouterState, treePatch) {
+    // disable soft navigation to solve issues with server side dynamic segments
+    // https://github.com/vercel/next.js/issues/42991
+    return true;
     const [segment, parallelRoutes] = flightRouterState;
     // TODO-APP: Check if `as` can be replaced.
     const [currentSegment, parallelRouteKey] = flightSegmentPath;

The steps to use it are:

install patch-package
add the script "postinstall": "patch-package"
Update /node_modules/next/dist/client/components/reducer.js as shown in the patch link
run yarn patch-package next

after this your app should work with Hard navigation only

focux commented 1 year ago

If you don't want to patch next, I'm using this workaround and it's working well so far.

startTransition(() => {
    router.push('/some-route')
   router.refresh()
});

zenflow commented 1 year ago

@focux that will request data twice in the case where hard navigation was used

Fredkiss3 commented 1 year ago

As a more elaborated work around that basically avoid the creation or weird patterns on the code i just patched Next.js to disable soft navigation completely

@Josehower does this disable hard navigation for even static pages ?

And i don’t know, but if you can patch the code, can you create a PR to next to submit it for the NextJs team to consider ? 🤔 just asking

zenflow commented 1 year ago

And i don’t know, but if you can patch the code, can you create a PR to next to submit it for the NextJs team to consider ? 🤔 just asking

I was thinking the same thing, if only to get some attention to the fact we want/need this fixed.. but on the other hand I think they would never go with this approach.. I think they would want to fix the feature they worked to introduce rather than disable it or blow it away even though personally I would love if they just blew it away

Fredkiss3 commented 1 year ago

What about disabling it only for dynamic pages ?

Josehower commented 1 year ago

@Josehower does this disable hard navigation for even static pages ?

unfortunately yes.

I am still researching on how to disable it only on dynamic pages, but it has been harder than expected. This is on my case just a way to solve it temporary while Next fix it. I really believe they need to fix it soon.

If i manage to fix it only for dynamic pages ill let you know

markitosgv commented 1 year ago

@Josehower @Fredkiss3 here the same! I want to force server side fetching again on some dynamic pages when user is navigating. I think is a normal behaivour to get "fresh" pages when user's are navigating between pages

Josehower commented 1 year ago

Issue still happening on v13.1.3 and in 13.1.4-canary.0

stackblitz.com/edit/github-6fhvxk?file=src%2Fapp%2Fpage.jsx

markitosgv commented 1 year ago

And I would like to add @Josehower a new test forking your stackblitz with soft navigation for a dynamic segment, that doesn't work either on v13.1.3 or 13.1.4-canary.0

Is exactly this case Hard Navigation

Navigatin to /dashboard/[team-x]/detail links seems to be a soft navigation always

So... I don't know if it's buggy at all @Fredkiss3

https://stackblitz.com/edit/github-6fhvxk-pyo7u7?file=package.json

Fredkiss3 commented 1 year ago

@markitosgv i've seen your example but i don't understand what you don't understand... ? It is definitely buggy i think ?

markitosgv commented 1 year ago

@markitosgv i've seen your example but i don't understand what you don't understand... ? It is definitely buggy i think ?

Sorry , the issue is navigating to /dashboard/[team-x]/detail links, in docs says that a hard navigation will occur, but it seems that is always a soft naviagtion too

psugihara commented 1 year ago

Are you guys also seeing an issue where router.refresh on the current page will not invalidate the router cache and subsequent soft navigations still show cached pages? Is that related to this? In my mind the better fix for this issue is making router.refresh reliably invalidate the router cache... when someone logs out or mutates data that should update on other pages, I should just be able to router.refresh and totally clear out the router cache. Then next can prefetch pages again, etc to make nav fast. If I understand, that's what the docs recommend and that aligns with my intuition about how a mutation should be handled.

The current behavior I observe is that router.refresh only invalidates the current page segments. So if I mutate data / log out, the soft navigations have stale data (e.g. I visit profile to see my "likes" then go to /detail-page/xyz where I "like" something then bac to /profile where I see my "likes" but it's missing the new one).

As a workaround I've replaced Links with DynamicLinks as described in this comment but it makes the navigation much slower than it could be with prefetching and loading states.

karlhorky commented 1 year ago

Are you guys also seeing an issue where router.refresh on the current page will not invalidate the router cache and subsequent soft navigations still show cached pages? Is that related to this?

Not sure, this sounds similar but not 100% related.

As far as I understand, this particular issue (#42991) is about next/link and router.push() selecting soft navigation for pages which have opted in to dynamic rendering using dynamic = 'force-dynamic' or revalidate = 0. This results in the page showing stale cached data, instead of running the logic in the async function component again and showing fresh data.

I described it also in the Next.js 13 app directory feedback discussion:

https://github.com/vercel/next.js/discussions/41745#discussioncomment-4732109

In my mind the better fix for this issue is making router.refresh reliably invalidate the router cache... when someone logs out or mutates data that should update on other pages, I should just be able to router.refresh and totally clear out the router cache. Then next can prefetch pages again, etc to make nav fast. If I understand, that's what the docs recommend and that aligns with my intuition about how a mutation should be handled.

The current behavior I observe is that router.refresh only invalidates the current page segments. So if I mutate data / log out, the soft navigations have stale data (e.g. I visit profile to see my "likes" then go to /detail-page/xyz where I "like" something then bac to /profile where I see my "likes" but it's missing the new one).

@psugihara so you're proposing this is related to mutations, eg. the "The Next.js team is working on a new RFC for mutating data in Next.js" statement that appears currently in the beta docs.

I can follow what you're saying, and it seems like maybe you're asking for this RFC to be released, with a proper solution for cache invalidation on other pages when something changes that will affect multiple pages. This would indeed be great, and would probably provide a solution for a lot of these use cases. (not all of them, still unanswered are time-based invalidations and other cache invalidations not based on a mutation - eg. as mentioned below by @Fredkiss3)

However, until that happens, if the Next.js team could provide an interim solution to disable soft navigation automatically for dynamic pages, that would be nice.

Fredkiss3 commented 1 year ago

It can be very useful to always have fresh data for navigation, even with mutations.

I have a use case in my company where we have an app (the app is not coded with nextjs, but bear with me) that has a short jwt token (about 1 hour), we want our users to be logged out after one hour without any mutation done, the app is a dashboard so it can be left in a tab without refreshing for a long time.

If every navigation can get fresh data, we won't have any problem with stale data and sessions.

karlhorky commented 1 year ago

Nice example, seems like the "admin dashboard" use case is a common theme here.

Fredkiss3 commented 1 year ago

That's normal, the most popular example use case for nested layouts is dashboard 😅

karlhorky commented 1 year ago

Another use case by @Albert-Gao:

do the cache invalidation from a different place, like update user cache only after user info updated

https://github.com/vercel/next.js/discussions/41745#discussioncomment-4771563

karlhorky commented 1 year ago

Looks like Tim wrote some tests for the shouldHardNavigate function to verify that it will soft navigate if the segments match:

https://github.com/vercel/next.js/pull/45303/files

So it's possible that this is a signal that Next.js does not plan to provide hard navigation for these cases...

So maybe there will indeed need to be a way that all use cases are supported by the changes provided in the future Mutations RFC

zenflow commented 1 year ago

So it's possible that this is a signal that Next.js does not plan to provide hard navigation for these cases...

I feel it might be different if maintainers were aware of this issue, but I don't think they are? There's been no comment from any of them here or the other duplicate issues that I'm aware of.

This is very concerning as I think this is a huge and obvious flaw in the specified/documented behavior and AFAICT there is no way to fix it without some kind of breaking change. I hope it gets some attention before declaring app dir stable.

Does anyone else feel like maintainers aren't aware of this issue and should be? Does anyone have any idea how to get their attention?

So maybe there will indeed need to be a way that all use cases are supported by the changes provided in the future Mutations RFC

I'm doubtful of that since I assume that change wouldn't (or shouldn't) affect behavior when we're not doing mutations, or when mutation happened in a different tab or by another user. For some pages we just want a navigation to always get fresh data. I think support for this would need to be a separate change.

Fredkiss3 commented 1 year ago

I think there are too much unresolved bugs for the team to focus on this specific one, seems like they are working on shipping features right now.

We have to be patient and not harass the team.

And if after a long time, this one is not resolved (and is closed automatically), we can still create a new one, by providing all the context needed.

karlhorky commented 1 year ago

Here's a new patch-package patch for next@13.1.6, since José's previous patch is broken now:

patches/next+13.1.6.patch

diff --git a/node_modules/next/dist/client/components/router-reducer/should-hard-navigate.js b/node_modules/next/dist/client/components/router-reducer/should-hard-navigate.js
index 150a5fd..8ccb52f 100644
--- a/node_modules/next/dist/client/components/router-reducer/should-hard-navigate.js
+++ b/node_modules/next/dist/client/components/router-reducer/should-hard-navigate.js
@@ -5,6 +5,7 @@ Object.defineProperty(exports, "__esModule", {
 exports.shouldHardNavigate = shouldHardNavigate;
 var _matchSegments = require("../match-segments");
 function shouldHardNavigate(flightSegmentPath, flightRouterState) {
+    return true; // Disable soft navigation for always-fresh data https://github.com/vercel/next.js/issues/42991#issuecomment-1413404961
     const [segment, parallelRoutes] = flightRouterState;
     // TODO-APP: Check if `as` can be replaced.
     const [currentSegment, parallelRouteKey] = flightSegmentPath;

Also in our example repo here: https://github.com/upleveled/next-js-example-winter-2023-vienna-austria/blob/d99ffd14608aa41ee082009259d657aad7e3f34a/patches/next%2B13.1.7-canary.7.patch

zenflow commented 1 year ago

We have to be patient and not harass the team.

Yeah, I agree. That would be ineffective and not appreciated. I was wondering if there's any other way to bring this to their attention. But I suppose it may just not be a priority.

timneutkens commented 1 year ago

Thanks for your feedback!

To provide an update here: I'm discussing the right caching behavior for links / refresh with Sebastian still, definitely listening to your feedback and we understand this particular case. A part of this will be how it integrates with mutations.

So it's possible that this is a signal that Next.js does not plan to provide hard navigation for these cases...

Please don't read things into "unit tests were added", you can always ask me. I've been working on adding unit tests to the router to ensure that how it works right now is covered and others on the team are able to ramp up on working on the new router, especially as it's affected by concurrent features (e.g. double-invokes etc) having these tests is incredibly important.

This is very concerning as I think this is a huge and obvious flaw in the specified/documented behavior and AFAICT there is no way to fix it without some kind of breaking change. I hope it gets some attention before declaring app dir stable. Does anyone else feel like maintainers aren't aware of this issue and should be? Does anyone have any idea how to get their attention?

Please be patient. There's a ton of different reports, including this one being duplicately reported across issues/discussions. We've been focused on ensuring all changes that would affect the code you're writing are landed, for example replacing head.js with the new metadata API, route handlers which replace api routes, many bugfixes to CSS handling, ensuring edge cases in the new router don't make applications hang, static generation / ISR improvements, MDX support, and more. A benefit of this is that now the majority of features and the way you write them has been landed, allowing us to focus on bugfixes and refining behaviors further. There's still some features coming, notably mutations, parallel routes, and interception.

karlhorky commented 1 year ago

Thanks for the update here @timneutkens, super appreciated! Looking forward to the Mutations RFC!

Please don't read things into "unit tests were added", you can always ask me

Ok, will try not to do this, sorry for the assumptions!

Josehower commented 1 year ago

Hi all,

currently the patch updated by @karlhorky here https://github.com/vercel/next.js/issues/42991#issuecomment-1413404961 is broken since 13.1.7-canary.18.

I have updated the patch in a way that only disable the hard navigation for next/link what is a bit less invasive. so functions as router.push() should still work with soft navigation. since there we have the router.replace() + router.refresh() trick.

the updated patch for 13.2.3 is:

patches/next+13.2.3.patch

diff --git a/node_modules/next/dist/client/components/layout-router.js b/node_modules/next/dist/client/components/layout-router.js
index 9b60a45..dd0639d 100644
--- a/node_modules/next/dist/client/components/layout-router.js
+++ b/node_modules/next/dist/client/components/layout-router.js
@@ -317,6 +317,7 @@ function HandleRedirect({ redirect  }) {
     const router = (0, _navigation).useRouter();
     (0, _react).useEffect(()=>{
         router.replace(redirect, {});
+        router.refresh()
     }, [
         redirect,
         router
diff --git a/node_modules/next/dist/client/link.js b/node_modules/next/dist/client/link.js
index d15ce7f..369e036 100644
--- a/node_modules/next/dist/client/link.js
+++ b/node_modules/next/dist/client/link.js
@@ -83,6 +83,7 @@ function linkClicked(e, router, href, as, replace, shallow, scroll, locale, isAp
     if (isAppRouter) {
         // @ts-expect-error startTransition exists.
         _react.default.startTransition(navigate);
+        router.refresh()
     } else {
         navigate();
     }

ayhanap commented 1 year ago

Doing router.refresh() on / does not clear the cache for /hello etc. The only way to prevent this seems to be prefetch=false atm. This was working differently on 13.1.x. Spent hours on this but there seems to be no way of managing the simple logic of clearing the cache for dynamically generated pages.

Watching several discussions and issues now, hope we can see some changes around this.

karlhorky commented 1 year ago

Seems like there will be a solution for invalidating the Next.js Cache for different routes programmatically in one of the next versions of Next.js:

Next up, we plan to implement programmatic updates to specific paths or cache tags.

https://vercel.com/blog/vercel-cache-api-nextjs-cache#:~:text=Next%20up%2C%20we%20plan%20to%20implement%20programmatic%20updates%20to%20specific%20paths%20or%20cache%20tags.

Fredkiss3 commented 1 year ago

@karlhorky That still means that we wil have to manually put revalidatePath('/dynamic-path'); to each path that we always want to be up to date, and i can't stop thinking that this is a hack.

And secondly, i think the revalidation will only work serverside right ? the hard navigation case happen in client.

And one thing i have noticed is that in the code, there is no concept of a dynamic rendering (i.e. with dynamic = "force-dynamic" or using dynamic functions like cookies & headers) only the concept of dynamic segment (i.e. /path/[dynamic]), furthermore even if the segment is dynamic, once it is in the cache next app router always do a soft navigation wether it opts into dynamic rendering or static rendering.

from here :

// packages/next/src/client/components/router-reducer/should-hard-navigate.ts

export function shouldHardNavigate(
  flightSegmentPath: FlightDataPath,
  flightRouterState: FlightRouterState
): boolean {
  // ... rest of the code

  // Check if current segment matches the existing segment.
  if (!matchSegment(currentSegment, segment)) {
    // If dynamic parameter in tree doesn't match up with segment path a hard navigation is triggered.
    if (Array.isArray(currentSegment)) {
      return true
    }

    // If the existing segment did not match soft navigation is triggered.
    return false
  }
   // ... more code
}

Fredkiss3 commented 1 year ago

Ok update, it does a hard navigation when navigating to a dynamic segment (/path/[dynamic]), i've tested it the latest canary :

https://stackblitz.com/edit/vercel-next-js-n1tqpr?file=app%2Fpage.tsx,app%2F[dynamic]%2Fpage.tsx,app%2Flayout.tsx,app%2Fdynamic%2Fpage.tsx

The only downside is that it does not do a hard navigation when using a static segment marked with dynamic = "force-dynamic".

vercel / next.js