vercel / next.js

The React Framework
https://nextjs.org
MIT License
126.94k stars 26.97k forks source link

[NEXT-1186] revalidatePath not working for dynamic routes while res.revalidate works fine #49387

Closed roxizhauk closed 1 year ago

roxizhauk commented 1 year ago

Verify canary release

Provide environment information

    Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 22.5.0: Mon Apr 24 20:51:50 PDT 2023; root:xnu-8796.121.2~5/RELEASE_X86_64
    Binaries:
      Node: 16.17.1
      npm: 8.19.2
      Yarn: N/A
      pnpm: N/A
    Relevant packages:
      next: 13.4.7-canary.1
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.3

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

App directory (appDir: true)

Link to the code that reproduces this issue

https://github.com/roxizhauk/revalidate-path CodeSandBox

To Reproduce

Visit /blog/[anything] (e.g. /blog/test ) to generate a dynamic page and check the time shown

To try "unsuccessful" app directory's revalidatePath:

  1. hit /api/revalidate-path?path=/blog/[anything] and you'll see "revalidated"
  2. refresh /blog/[anything] and you'll see the time not changed

To try "successful" pages directory's res.revalidate:

  1. hit /api/revalidate?path=/blog/[anything] and you'll see "revalidated"
  2. refresh /blog/[anything] and you'll see the time changed

Describe the Bug

The app directory's revalidatePath works fine for "/" or "/blog" but dynamic routes like "/blog/1" or "/blog/test" while pages directory's res.revalidate works fine for all

Expected Behavior

revalidatePath works the same as res.revalidate for dynamic routes

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

From SyncLinear.com | NEXT-1186

robmarshall commented 1 year ago

@jwalcher Myself as well. I have spent the last 2 days (and still) trying to do what res.revalidate did. With no success.

markomitranic commented 1 year ago

I stumbled upon Vercel's take on how to do this with tags :) https://github.com/vercel/commerce/blob/main/app/api/revalidate/route.ts

@leerob The issue isn't that we don't understand the intent of the approach, instead it is the cardinality of it - it isn't very sane to revalidate 7000 product pages every time any product changes. We need to be more granular with this, and the old uri-based API allowed that out of the box. Now, we either have to make it ourselves, which is a pita, or use pages router for this singular purpose.

I agree that the tags approach is healthier in the long run, and able to tightly integrate cache busting with next. Its just a pain to write granular tagging mechanisms ourselves, that is all. So I'm kinda hoping to see a package/utility pop up that would offer a better devex when using tagging.

leerob commented 1 year ago

Sorry if this wasn't clear – Route Handlers are revalidate are stable. I'm referring your comment about unstable_cache().

So I'm kinda hoping to see a package/utility pop up that would offer a better devex when using tagging.

This is what unstable_cache(), when stable, will do!

levipadre commented 1 year ago

Any update on how to revalidate a single page? At the moment (using v13.4.16) it still revalidates all the children of the revalidated page too, which is not ideal.

Galanthus commented 1 year ago

Any update on how to revalidate a single page? At the moment (using v13.4.16) it still revalidates all the children of the revalidated page too, which is not ideal.

How do you revalidatePath atm? revalidating doesn’t work for me on Vercel.

jwalcher commented 1 year ago

I recently implemented tagging all fetches and calling revalidateTag via webhooks from my CMS, to get a fine-grained revalidation at the level of individual pages. However, leaving aside the extra work, I would still argue that at the moment it does not work according to specs, as documented in https://nextjs.org/docs/app/building-your-application/caching#data-cache

Namely, in production, after revalidateTag is executed on the server, we need two client requests to see the new content. According to the docs, the cache should be purged immediately, and the new content show on the first client request. (In dev-mode, one request is indeed sufficient.)

If anyone can confirm this diagnosis, we should perhaps file a new bug.

levipadre commented 1 year ago

Could you share your code @jwalcher? Just to compare

jwalcher commented 1 year ago

app/[category]/page.js

export default async function CategoryPage({ params }) {
    const endpoint = params.category
    const pageData = await fetch(`${global.fetchURI}${endpoint}`, {
        next: { tags: [params.category] }
    })
return <> {pageData} </> 
}

app/api/revalidate/route.js

const revalPayloadToTags = (payload) => { 
return [payload.category]
}

export async function POST(request) { 
  const payload = await request.json();
  const tags = revalPayloadToTags(payload);
  for (let tag of tags) { revalidateTag(tag); }
   return NextResponse.json({ revalidated: true }, { status: 200 });
}
kavehsaket commented 1 year ago

I'm using 13.4.19 and revalidate path doesn't work for nested routes however my revalidate route handler return 200. @leerob is this api stable? Thanks

roxizhauk commented 1 year ago

What I currently understand (though it's merely a re-phrase of https://github.com/vercel/next.js/issues/49387#issuecomment-1552394378) is:

So I guess we just use them separately for now until they deprecate pages/

ijjk commented 1 year ago

Hi, the revalidatePath() behavior has now been fixed to correctly revalidate specific URLs instead of just page paths in v13.4.20-canary.27 of Next.js, please update and give it a try!

An example of the issue that was fixed here, when you only want to revalidate /blog/post-1 you can now pass that to revalidatePath instead of having to invalidate all paths for /blog/[slug]/page.

DennieMello commented 1 year ago

Hi, the revalidatePath() behavior has now been fixed to correctly revalidate specific URLs instead of just page paths in v13.4.20-canary.27 of Next.js, please update and give it a try!

An example of the issue that was fixed here, when you only want to revalidate /blog/post-1 you can now pass that to revalidatePath instead of having to invalidate all paths for /blog/[slug]/page.

Thank you! I confirm that now both updating the cache of one page and updating the cache of the entire route works, using the second argument "page". I just suggest adding to the documentation a variation using routing groups. If you need to clear the entire route segment - revalidatePath("/(main)/post/[slug]","page"). This is not very obvious, given that to clear one page you need to set evalidatePath("/post/post1).

I'm really glad this problem is finally fixed. It would also be great if you could help bring the developers' attention to another cache-related issue that has been active for over 6 months. If the page has dynamic parameters, the 404 error is returned only the first time the page is rendered in production. All the following responses return a 200 response. #43831 #48342 #51021 #45801

ijjk commented 1 year ago

@DennieMello thanks for the flag, opened https://github.com/vercel/next.js/pull/55542 and https://github.com/vercel/next.js/pull/55543 addressing those.

jwalcher commented 1 year ago

Thanks @ijjk! Now trying to update from 13.3.0 to 13.5.1, two comments:

  1. Tests with revalidatePath on dynamics routes are encouraging. One remaining confusion is the timing of cache invalidation. According to the general docs, one would expect that the cache should be cleared immediately when calling revalidatePath. The API description however says that this will only happen when the path is next visited. This is the behavior I'm seeing, at least in production we have to refresh the routes twice before seeing the new data. That's not quite optimal in my mind.

  2. My production app uses a third-party library (apollo graphql) for data fetching. On demand revalidation might or might not work, but for the time being I am getting a “Failed to set fetch cache TypeError: fetch failed on every fresh build. Is that expected or is the entire caching mechanism not yet working for third party libraries (and we have to wait for the unstable_cache api)? (fwiw, the trouble seems to start at 13.4.13, which is also when res.revalidate works again for me)

I'll keep trying, does either of these sound worthy of a bug report?

karlhorky commented 1 year ago

For point 2, there are these issues related to TypeError: fetch failed, maybe related:

jwalcher commented 1 year ago

Looks related indeed. Funny thing over here is that if I first build in an old version (say 13.4.12), upgrade to 13.3.13 and then build again, there's no error. It only appears after I delete .next/cache and build fresh.

Update: Most closely related issue to second point is https://github.com/vercel/next.js/issues/53695, not directly related to revalidation

Update: Indeed, issue 2 can be fixed by other means. I'm still curious about 1 though, when exactly revalidation is supposed to happen.

DennieMello commented 1 year ago

@DennieMello thanks for the flag, opened #55542 and #55543 addressing those.

Thank you for fixing the error so quickly!

When using revalidatePath(/(main), "page") to clear the main page cache, the cache of all pages located in (main)/page/[page]/page.tsx is also cleared along with it. In other folders that are not named "page", the cache is not cleared. So the problem seems to be caused by a comparison with the name "page". I think this is a special case and this problem clearly does not require an urgent fix, I just noticed non-obvious behavior.

roxizhauk commented 1 year ago

@ijjk Dynamic routes visited through <Link> won't be updated when using revalidatePath whereas res.revalidate works as expected. Seems like the cache in the client-side (Router Cache?) somehow shows the same page. You can still use <a> tags to make it work, but yet I'm wondering if this is intended.

madfcat commented 1 year ago

Hi, the revalidatePath() behavior has now been fixed to correctly revalidate specific URLs instead of just page paths in v13.4.20-canary.27 of Next.js, please update and give it a try!

An example of the issue that was fixed here, when you only want to revalidate /blog/post-1 you can now pass that to revalidatePath instead of having to invalidate all paths for /blog/[slug]/page.

The problem is still here. But in a smaller effect.

I've been testing revalidatePath with different parameters on v13.5.3. For example, revalidatePath("/(main)/cases/[slug]", "page") worked fine.

But when I tried to

const path = `/cases/${slug}`;
revalidatePath(path);

it worked only when I was refreshing and staying on the exact same page. Coming from the other page or switching to other page made the cache stale and I could not revalidate anymore. So I had to rebuild the whole project. The bad thing that it's hard to figure out visiting which page makes the cache impossible to revalidate.

I can revalidate the page just putting the url in the browser. It works many times in a row. But when I start to go between the pages, at some point revalidation stops working.

I am using res.revalidate approach from pages router which at least works. I could not understand at which point App router revalidatePath stops working. The issue is still there when trying to revalidate a specific path from the dynamic route.

sahanxdissanayake commented 1 year ago

This issue still persists

picozzimichele commented 1 year ago

same here, revalidatePath() for dynamic routes not working

using "next": "13.5.4"

khristovv commented 1 year ago

Experiencing the same issue as well. revalidatePath doesn't seem to be working correctly. The only page/route I'm able to revalidate seems to be the root /. Revalidate calls to dynamic routes such as posts/post-1 don't seem to be working.

Using version: 13.5.4

wencakisa commented 1 year ago

Same here. Any revalidatePath calls to dynamic routes such as revalidatePath("posts/1") don't seem to be working as expected.

Using version: 13.5.4

picozzimichele commented 1 year ago

Found the issue in my code @khristovv. My UI was not updating correctly, however if I logged on the server whatever is returned from the serverAction, it was updating correctly. Try to log the returned value from your API and see if it changes correctly, if it does then revalidatePath() is working well, the issue might be somewhere else.

I am using a dynamic route /dashboard/properties/[id]

1 - Getting the propertyDetails in a async server component.

propertyDetails = await fetchProperty({ propertyId: params.id });

2- passing the details from server component to -> client component and calling the updatePropertyDetails with revalidatePath from the client component.

3- The UI now is updating correctly as long as I only use the data coming directly from the propertyDetails from the server component, without changing the data

Scrap my old comment, seems to be working correctly, my mistake was assigning the propertyDetails to a new variable in the client component, try to use the return value directly

maksimgorodilov commented 1 year ago

Hello, I'm using 13.5.4 version

Getting the same problem as people above. app revalidatePath does not work, but pages res.revalidatePath does work.

I have a simple page in /app/static-page/page.tsx

export default () => {
    return (
        <div>Hello {Date()}</div>
    );
}

Then I have app revalidate route in /app/api/update-static-page/route.ts

import {revalidatePath} from "next/cache";
export async function GET() {
    revalidatePath('/static-page', "page");
    return Response.json({ revalidated: true })
}

And I have pages revalidate route in /pages/api/revalidate.ts

import { NextApiHandler } from "next";

const handler: NextApiHandler = async (req, res) => {
    try {
        await res.revalidate('/static-page');
        return res.json({ revalidated: true });
    } catch (err) {
        return res.status(500).send("Error revalidating");
    }
};

export default handler;

Then I run with rm -rf ./.next && npm run build && npm run start (clearing .next, just to make sure there is no conflicts from previous builds)

Then if I call app revalidate route, the static page does not change, but if I call pages revalidate route, it does change.

github-actions[bot] commented 1 year ago

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.