sanity-io / next-sanity

Sanity.io toolkit for Next.js
https://www.sanity.io/
MIT License
752 stars 91 forks source link

revalidateTag() issues #639

Closed thedevdavid closed 8 months ago

thedevdavid commented 1 year ago

I’m trying to get next app router + revalidateTag() to work with Sanity but it’s either completely unpredictable or I totally misunderstood something.

I now spent 5 full days trying to make it work and build multiple projects. Here’s what I can tell now:

Project 1: This is the most basic, simple setup (posts, pages, categories, [slug] dynamic pages). I think this works. https://github.com/thedevdavid/sanity-revalidate-debug

Project 2: Almost the same setup (posts, pages, categories, [slug] dynamic pages + subpages with extra tags). Here, the simple tags: [job] or tags: [benefit] requests are never getting revalidated on the / and /post-a-job routes. But in (for example) /contract/[slug] route. And the complex tags, like [job:${params.slug}] or [page:${params.slug}] are all working well.

Projects 3 & 4: Very similar to project 2. posts, pages, and categories with extra schema types, like team members, navigation links, testimonials, and more. Here I can’t even tell the exact problems. Probably similar to project 2 but I’m not sure anymore.

Although there’s 1 thing I know that’s extra. There’s a homepage type with the fetch tag, homepage. This type has the navigation links. There’s a fetch tagged with homepage on (site)/page.tsx and in 2 components (Navbar & Footer). Both are rendered from (site)/layout.tsx. The fetch from the components gets revalidated and I can see the new data on all pages, except the / route, aka. (site)/page.tsx. That route never gets revalidated. Although the page and 2 components inside have tagged requests.

In all 4 projects, the Sanity webhook calls the revalidate API route. The function always succeeds. Params are what they are supposed to be, so I guess the revalidateTag() gets called with the right parameters. (Although I have not figured out how to debug the actual function calls yet)

Soooo I feel totally lost and I haven’t even tried to include previews and draft mode yet.

I'm not sure if it's a bug with Sanity, Next.js, or it's just me doing something wrong.

stipsan commented 1 year ago

Hi David! Thank you for the detailed report, we'll get to the bottom of this 🙌

stipsan commented 1 year ago

OK so there seems to be a problem with the segment itself and perhaps not the fetch cache. I see you have experimental logging enabled, it would be interesting to see if it reports "HIT", "MISS" or "SKIP" for the routes that never revalidate.

And if you go to your Vercel Project settings: https://vercel.com/:teamIdorSlug/:projectIdOrSlug/settings/data-cache And hit "Purge Everything" image does that make any difference?

If no, then your client setup is correct and we do indeed need to look at the segment and page routing and see if Next.js thinks it's perhaps a static route. But it should be treating it as a dynamic ISR route 🤔

If yes then the next steps is to look at sanityFetch and update it with new findings we've made in R&D as our team at Sanity have been using App Router a lot in the field over the summer. Some of our assumptions have turned out to be false, and our current docs and learning materials are lagging behind. For example we're moving away from defining fetch.cache: "force-cache" | "no-store" to next.revalidate: false | 0 and we're working on new materials and docs.

thedevdavid commented 1 year ago

And if you go to your Vercel Project settings: https://vercel.com/:teamIdorSlug/:projectIdOrSlug/settings/data-cache And hit "Purge Everything" does that make any difference?

Nope. I changed a post's title and refreshed the website with cmd+shift+R. No changes on the home page. Then went ahead and "purged everything". Back to the website, and refresh a couple of times. Still the previous title. Navigating to /job/[slug] loads the new title. Navigating back to /, still the previous title. After refresh, still the same. The data only refreshes after redeployment.

Here's a list of requests when I modify a post title:

image

Aside from the one selected, all others are cache HITs.

stipsan commented 1 year ago

Gotcha, could you share the build output and how routes are determined? Example: image

Then could you try and force dynamic rendering in the index route page.tsx? Seems like ISR isn't working correctly, App Router should revalidate it in the background and update the full page cache. We can test this theory by forcing the route to be dynamic instead of the default static + ISR behavior.

Here's how:

export const revalidate = 0

This makes the layout dynamic, and changes the default cache behavior on fetch calls from force-cache to no-store, but that shouldn't affect you as your API calls are setting cache explicitly.

Note that if you use this instead:

export const dynamic = 'force-dynamic'

It overrides all fetch calls, not just changing defaults. Should the revalidate segment config not work you may use dynamic as a blind test. If dynamic doesn't work then there's a Next.js bug in play.

In any case, I think we may have to call in the cavalry over at https://github.com/vercel/next.js/issues/newto get to the bottom of this 😅

thedevdavid commented 1 year ago

Sure. See my build output here:

image

Adding export const revalidate = 0 sets the / route to server.

image

Also, deployed with revalidate = 0 does update the data but for some reason, api/revalidate gets called 2x. I only changed the title of a post.

image
stipsan commented 12 months ago

Ok I see. I think the course of action is to reach out to vercel over on the next.js repo and let them take a look.

Regarding the 2x requests, that can sometimes happen as our webhook delivery system is distributed and optimized for speed. And is why they have an idempotency-key header to aid in deduping them for use cases where each webhook has to be processed exactly once per event: https://www.sanity.io/docs/webhooks#3e9b7dac38b7 In your case it's irrelevant, as the /api/revalidate doesn't cause a WRITE to the cache. They mark the cache as stale so on the next fetch, instead of a HIT you'll get a MISS that will write to the cache after a successful fetch.

Galanthus commented 10 months ago

@stipsan Is there like a work-around for now. I am also facing the same issue with the starter you have worked on using the app router. It would revalidate page, post, project etc anything that contains a slug. But having document "category" for instance if that's a reference set to the page, and if you directly change the category name won't revalidate. I first have to save that same page again in order to revalidate. Is this something familiar? Basically, you have to change the "Page" again with no changes or whatsoever in order to change the document "category" name. Changing the category won't revalidate. Even if I provided it to the "tags" array, Vercel logs return revalidated and Webhook returns code 200.

Is there a fix or workaround?

Would highly appreciate if you can reply to this.

Thanks in advance.

This is the snippet from the starter which I've extend which doesn't work:

export function getSettings() {
  return sanityFetch<SettingsPayload>({
    query: settingsQuery,

    tags: ["settings", "home", "page", "post", "project", "faq", "category", "service"]
  })
}
mitchatinvestin commented 10 months ago

Obviously loathe to "me too" this, but your initial post is absolutely correct. Any basic tag (['posts'] or ['faqs']) does not work, but more specific ones (['post:${slug}']) do.

This discrepancy kind of leads me towards thinking its a Sanity issue, as NextJs shouldn't really make any distinction between 2 types of tag, right?

mitchatinvestin commented 10 months ago

For reasons I'm not even going to pretend to understand, putting an underscore in my tag (e.g posts_ instead of posts) seems to fix it in most instances, but doesn't for some. What the hell is going on here 😂

deckchairlabs commented 10 months ago

Also experiencing this same issue

chandlervdw commented 10 months ago

I am also seeing this where _type:slug is updating but _type is not. In my case, ['blog:${slug}'] is updating but ['blog'] is not. I have both revalidateTag() calls in my endpoint:

    revalidateTag(body._type)

    if (body.slug) {
      revalidateTag(`${body._type}:${body.slug}`)
    }
Stelkooo commented 10 months ago

I am also seeing this where _type:slug is updating but _type is not. In my case, ['blog:${slug}'] is updating but ['blog'] is not. I have both revalidateTag() calls in my endpoint:

    revalidateTag(body._type)

    if (body.slug) {
      revalidateTag(`${body._type}:${body.slug}`)
    }

I have the opposite issue where ['blog'] is updating while ['blog:${slug}'] is not. Also for me, having multiple tags does not work either so that's why I am sticking to just the one.

YCMitch commented 10 months ago

Chiming back in here to say on NextJs 14 it doesn't appear to work at all. One tag, multiple tags, with slug, without slug. Just doesn't clear anything unless I go and purge it all myself.

If we swap over to WordPress with WPGraphQL it all works exactly as expected, so I'm pretty 100% it's a Sanity issue. We're gonna go back to WP until this is working as it's a pretty big dealbreaker.

Stelkooo commented 10 months ago

@MitchEff, I have a Next.js 14.0.2 app and its working for me with a single tag.

YCMitch commented 10 months ago

Update: updating to NextJs canary at least now can clear the cache provided I use one tag...

chandlervdw commented 10 months ago

This is related to https://github.com/vercel/next.js/issues/55960

Galanthus commented 10 months ago

Update: updating to NextJs canary at least now can clear the cache provided I use one tag...

It was functional even before with a single tag and revalidation. However, the primary challenge arises when attempting to revalidate multiple tags. The cache doesn't seem to clear properly in such cases, either firing too late or too early.

williamli commented 10 months ago

The latest Next canary (v14.0.4-canary.28) fixed the issue for me. I am able to revalidate 4 tags without issues.

r4zendev commented 9 months ago

Next.js v14.0.5.canary-16 next-sanity v7.0.4

My server component fetching a list of reviews to display them:

export default async function Product({
  params: { id },
}: {
  params: { category: string; id: string };
}) {
  const productReviews = await getProductReviews(id);

My client component for creating a review record using react-hook-form:

  const form = useForm();
  const onSubmit = async (data, e) => {
    e?.preventDefault();

    try {
      await createReview({ id, username: user?.username ?? undefined, ...data });
    } catch (err) {
      toast({
        title: "Error",
        description: (err as Error).message,
        variant: "destructive",
      });
    }
  };
<Form {...form}>
  <form onSubmit={form.handleSubmit(onSubmit)} className="space-y-4">
    ...
  <Button
    type="submit"
    className="w-full sm:w-auto"
    isLoading={isSubmitting}
  >
    Submit review
  </Button>
  </form>
</Form>

My server action:

"use server";

export async function createReview(
  data: Infer<typeof ReviewFormSchema> & { id: string }
) {
  await submitReview(data);
  revalidateTag(REVIEWS_REQUEST_TAG);
}

My Sanity client query function:

export async function sanityFetch<QueryResponse>({
  query,
  params,
  tags,
  cache,
}: {
  query: string;
  params?: QueryParams;
  tags: string[];
  cache?: RequestCache;
}): Promise<QueryResponse> {
  return sanityClient.fetch<QueryResponse>(query, params, {
    cache: cache ?? "force-cache",
    next: {
      //revalidate: 30, // for simple, time-based revalidation
      tags, // for tag-based revalidation
    },
  });
}

export const REVIEWS_REQUEST_TAG = "get-product-reviews";
export async function getProductReviews(id: string): Promise<Review[]> {
  await sanityFetch({
    query: groq`*[_type == "review" && item->_id == $id]{
      _id,
      _createdAt,
      title,
      content,
      rating,
      username,
      userId,
    } | order(_createdAt desc)`,
    params: { id },
    tags: [REVIEWS_REQUEST_TAG],
  });
}

Upon creating a review record in Sanity, the revalidateTag called in server action does nothing. After refreshing the page, the data might become even older. Same happens if I set cache: 'no-cache'. Only after 2nd update, I'm able to see the updated data. I will try to find time to create a small repro. In the meantime, with this setup, I couldn't get the cache revalidation to work. Debug messages in Next.js console logs show that cache MISSes because of hard-refresh, but not new data is being displayed/request for reviews remains stale and contains old data.

UPDATE: setting { useCdn: false } actually resolves the issue and cache is back to normal. Is this the intended behaviour? Is there a way to improve the library to actually invalidate the cdns?

simonhrogers commented 8 months ago

This isn’t working for me either – even with an unmodified clone of your template repo: https://github.com/sanity-io/template-nextjs-personal-website.

It’s untenable to use at the moment, yet app directory is ready for production and users of other CMS report having no issues with revalidateTag(); furthermore all the respondents on the Next issues appear to be Sanity users. Can we therefore acknowledge it likely is an issue with Sanity, or at least something the Sanity team would need to be working with Next to fix?

This issue was opened in September and its unresolved over 3 months later. Do you think it would be possible to resolve this quite urgently?

@stipsan sorry to tag you, but what do you think? This is essentially a flagship starter for the Sanity product and remains unusable in production in the manner it proposes to be used. As it seemed to all be in order initially, I am now currently scrambling to investigate temporary alternatives having used this on a job that I must handover to client for content editing.

stipsan commented 8 months ago

@r4zendev

UPDATE: setting { useCdn: false } actually resolves the issue and cache is back to normal. Is this the intended behaviour? Is there a way to improve the library to actually invalidate the cdns? Yes, when using revalidateTag you can't use useCdn: true or you'll risk caching stale content. The CDN can have a TTL of up to a minute after content changes.

@simonhrogers

This isn’t working for me either – even with an unmodified clone of your template repo: https://github.com/sanity-io/template-nextjs-personal-website.

It’s untenable to use at the moment, yet app directory is ready for production and users of other CMS report having no issues with revalidateTag(); furthermore all the respondents on the Next issues appear to be Sanity users. Can we therefore acknowledge it likely is an issue with Sanity, or at least something the Sanity team would need to be working with Next to fix?

This issue was opened in September and its unresolved over 3 months later. Do you think it would be possible to resolve this quite urgently?

@stipsan sorry to tag you, but what do you think? This is essentially a flagship starter for the Sanity product and remains unusable in production in the manner it proposes to be used. As it seemed to all be in order initially, I am now currently scrambling to investigate temporary alternatives having used this on a job that I must handover to client for content editing.

The revalidateTag issues mentioned in this thread has been resolved, and we are using revalidateTag in production on our main website: www.sanity.io, as well as multiple mission critical enterprise customers that all rely on the GROQ webhook + revalidateTag setup. We weren't aware that the Personal Website template has a broken revalidateTag setup, I'll work on a fix for that and @-at you in the PR on that repo with the fix. It seems to be the webhook (app/api/revalidate) handler itself that fails to run at all, the webhook payload log reports 405 errors and there's nothing in our vercel logs.

I'm closing this issue for now, as the original issue is resolved and nowadays the problems we see with revalidateTag are typically: