vercel / next.js

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

App router deduping not working as expected #52126

Open richie-south opened 1 year ago

richie-south commented 1 year ago

Verify canary release

Provide environment information

Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 22.3.0: Mon Jan 30 20:42:11 PST 2023; root:xnu-8792.81.3~2/RELEASE_X86_64
    Binaries:
      Node: 16.13.0
      npm: 9.6.4
      Yarn: N/A
      pnpm: N/A
    Relevant packages:
      next: 13.4.7
      eslint-config-next: 13.4.7
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.6

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

No response

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

https://github.com/richie-south/nextjs-cache

To Reproduce

Clone the provided repo, run commands, check terminal logs for both api and next.

  1. start api server npm run api
  2. start next npm run dev

Describe the Bug

Next will make multiple fetch requests to the same path on page reload even if docs says they should be deduped.

This happens on dev and production deployment. if i reload the root page / i can see in my api logs that it will makes 3 request to my api, having fetch in my RootLayout and generateMetadata.

If i make a reload on other pages, ex /todo in my provided example, i will get 5 api requests for the same path.

Expected Behavior

I expect the requests to be deduped as illustrated in the docs. So instead of up to 5 requests it should be 1 when no cache is available.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

Other platform

NEXT-1407

zwarunek commented 11 months ago

Any update/quick fixes for this issue? I am experiencing a very similar thing

richie-south commented 11 months ago

This is still happening on 13.4.16 btw, and is causing large performance and costs for people not aware it is happening.

@zwarunek We got around this issue by manually wrapping every fetch with the cache function documentation.

franzwarning commented 11 months ago

Isn't this a massive issue? I'm kinda confused how everyone is experiencing this and nobody seems to be talking about the fact that deduping doesn't really work...

richie-south commented 10 months ago

Still happening on 13.5.3

raufdean commented 10 months ago

Yes a bit annoying. We're caching the promises from the fetch in a map keyed by url and purging regularly.. Luckily not a huge throughput, so not too much of an issue.

rjsdnql123 commented 9 months ago

Has this been resolved? I tried to reproduce the issue, but it appears that the server request is only occurring once in reality.

richie-south commented 9 months ago

Has this been resolved? I tried to reproduce the issue, but it appears that the server request is only occurring once in reality.

It is not fixed. You can check my example and view the server logs. There you will see multiple api calls to the server

rjsdnql123 commented 9 months ago

Hello, @richie-south c. May i handle this issue? Please assign to me. I'll create PR.

richie-south commented 9 months ago

@rjsdnql123 The issue is open for anyone to fix if they have the time :)

injae-kim commented 9 months ago

https://github.com/vercel/next.js/blob/218d0709eb97017e5bbd55f603bce033206bf71e/packages/next/src/server/lib/patch-fetch.ts#L501-L504

Hi @ztanner ! (author of incremental-cache at #53030) I'm trying to solve this issue with @rjsdnql123 . We have question about lock() on incremental-cache!

On patch-fetch.ts#L502, request try to lock incremental-cache first. When incremental-cache is empty and multiple requests with same url(cacheKey) comes in, these requests can get lock incremental-cache at the same time?

https://github.com/vercel/next.js/blob/218d0709eb97017e5bbd55f603bce033206bf71e/packages/next/src/server/lib/incremental-cache/index.ts#L241-L252

On above code, I think createLockIfAbsent logic is not synchronized over multiple requests so maybe multiple requests can create lock at the same time? 🤔

yarikpetrenko commented 8 months ago

Having the same issue. Did anyone found workaround or something? From what I'm seeing wrapping fetch in cache may be the possible workaround... After a lot testing I no longer understand when it breaks and how it works. Somehow my project works fine on Next.js 13.5.4 and below, and after uploading to Vercel it stays the same, but all versions above just don't dedupe fetch requests. Most frustrating thing that when I do all the same on codesandbox it just didn't work whatever version I choose request are not getting deduped, is so unpredicted and buggy. Because of that I can't upgrade to Next 14.

I'm surprised how little people care about it, there is only few issues and they don't have any responses. How people even use Next then. It completely breaks app router paradigm with many small server component where you can fetch your data and don't worry about duping.

In my case I have strict rate limit which I can't break and with this bug it just break all completely. It should be fixed ASAP.

I reproduces this issue on CodeSandbox for anybody who don't want to clone git repository to test this issue.

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

https://codesandbox.io/p/sandbox/gallant-tree-4yxvwz

To Reproduce

  1. Refresh the page
  2. In console you will see two timestamp with different time

Expected Behavior

  1. Refresh the page
  2. In console you will see two timestamp with the same time because second request was deduped and they share only one response.
psikoi commented 8 months ago

Having the same issue. Did anyone found workaround or something? From what I'm seeing wrapping fetch in cache may be the possible workaround... After a lot testing I no longer understand when it breaks and how it works. Somehow my project works fine on Next.js 13.5.4 and below, and after uploading to Vercel it stays the same, but all versions above just don't dedupe fetch requests. Most frustrating thing that when I do all the same on codesandbox it just didn't work whatever version I choose request are not getting deduped, is so unpredicted and buggy. Because of that I can't upgrade to Next 14.

I'm surprised how little people care about it, there is only few issues and they don't have any responses. How people even use Next then. It completely breaks app router paradigm with many small server component where you can fetch your data and don't worry about duping.

In my case I have strict rate limit which I can't break and with this bug it just break all completely. It should be fixed ASAP.

I reproduces this issue on CodeSandbox for anybody who don't want to clone git repository to test this issue.

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

https://codesandbox.io/p/sandbox/gallant-tree-4yxvwz

To Reproduce

  1. Refresh the page
  2. In console you will see two timestamp with different time

Expected Behavior

  1. Refresh the page
  2. In console you will see two timestamp with the same time because second request was deduped and they share only one response.

You can wrap the API requests with React.cache instead.

https://react.dev/reference/react/cache

const getData = cache(async () => {
  const res = await fetch("https://worldtimeapi.org/api/timezone/UTC", {
    credentials: "include",
  });
  return res.json();
});

I tried this on your codesandbox and it returns the same timestamp twice, and only sends the API request once, as you expected.

yarikpetrenko commented 8 months ago

@psikoi Thank you for your replay. I just tested your suggestion and it is indeed working🔥🔥.

For other people for more information you can check link that @psikoi provided and also Next docks - here From what I'm seeing you can just wrap your function with react cache function and it will "dedupe" all the requests you make with function you wrapped. It's great workaround. Also with this you need to move all your fetching logic so every component uses the same function and don't create own.

But I still can't comprehend how Next dev team or someone else still didn't fixed it, If I remember correctly they said that under the hood it takes advantage of react cache and it is not clear why it's not working.

OlegLustenko commented 8 months ago

Folks just for my curiosity for the ones who having an issue with this, are you using some fetch libraries?

So I've tried a few things and it was OK for specific cases, but I've used pure fetch here.

I want to check if this could be somehow related to bad references

yarikpetrenko commented 8 months ago

@OlegLustenko

Next.js extends the native fetch Web API to allow you to configure the caching and revalidating behavior for each fetch request on the server. React extends fetch to automatically memoize fetch requests while rendering a React component tree.

Docks👆

Docs says that dedupe works only if you use native fetch which Next.js extends. So yeah I'm personally experiencing things with pure fetch and didn't expect the same from libraries. And if you want to get this with third-party libraries it is recommended to use cache function.

externl commented 8 months ago

I'm seeing this specifically when using fetch with a cookie header.

OlegLustenko commented 8 months ago

Just in case, this is not an issue with prefetching it's pretty much a workaround

cpotey commented 7 months ago

Just to chime in, experiencing the same here. Currently wrapped a load of our fetch functions in cache - however it doesn't always use the cached versions, getting fresh data sometimes. Unsure whether cookies are the cause, but we require passing cookies into the fetch request headers for our setup.

Umming and ahhing whether to pivot to React Query to handle some of the caching/memoization it provides without the Next's fetch faff. Currently using it for client-side requests, but could look to use it for prefteching/SSR too.

My experience with React Query has been far more positive (and the devtools/visualising side) than Next's fetch, and made me feel far more confident in know what's cached/deduped etc

yarikpetrenko commented 7 months ago

@cpotey In my case cache function works fine as I wrote it earlier. Maybe your cookies change or something so it sends another request? Also from what I see on my production server fetch dedupe works fine. Only when I start it in dev mode fetch dedupe does not work or almost does not work. But when I build and start it works as expected for me...

cpotey commented 7 months ago

@cpotey In my case cache function works fine as I wrote it earlier. Maybe your cookies change or something so it sends another request? Also from what I see on my production server fetch dedupe works fine. Only when I start it in dev mode fetch dedupe does not work or almost does not work. But when I build and start it works as expected for me...

That's a good point. I'll try and specifically define the cookie I require (currently passing through all of them like Cookie: cookies().toString(),)

I'm seeing same things in dev/prod. I've got a console.log within my cache fetch requests, which is logging if the request is fetching fresh, rather than from cache - so can tell what's not deduping currently

yfs-2000 commented 6 months ago

14.0.4 This problem still exists

michalcizek commented 6 months ago

Hi, I am struggling with the same problem, this is a huge issue since the project uses a CMS Cloud with limits on API requests. Some not deduped fetches in layout happen like 10 times when combined with preloading links in view (the default behaviour) on a SINGLE page load.

I am using the graphql-request library, I have wrapped it with the cache function as suggested, but the problem persists:

export const cmsClient = new GraphQLClient(`${BASE_URL}/graphql`, {
  fetch: cache(async (input: RequestInfo | URL, params?: RequestInit) =>
    fetch(input, { ...params, next: { revalidate: REVALIDATE_TIME } })
  ),
})

Has anyone found a fix for this or should I try downgrading to a lower version? Could the library be a problem? I am passing it the fetch function from Next.js.

Edit: Now that I am looking at it, this might be a problem with the data cache and not request memoization, where the preloading of links triggers different server requests, and the memoization is not applicable. From the React docs: At this time, cache should only be used in Server Components and the cache will be invalidated across server requests.. However, shouldn't Next.js data cache handle this even across different server requests? The ones that are duplicately called have few milliseconds between them, which makes me think that before the request is completed and the response data is cached, another requests can still be called. Related to #47070

richie-south commented 5 months ago

I cannot reproduce this issue anymore with next 14.1.0 and my example project. Would be great if someone else wants to try, otherwise i will close this in the future. 😃

mikechabot commented 4 months ago

@richie-south

I have multiple RSCs wrapped with suspense, and the BE is still getting hit multiple times -- no de-duping is occurring. Moreover, there seems to be a cap on the number of parallel requests.

Per the simple logging below, you'll note the last two server component fetches take 10+ seconds (this is visible in the devTools waterfall below), however the server itself responded quickly to these last two requests, but only received the requests 10+ after Next began rendering. What is happening here?

$ node --version
v18.17.0

$ pnpm --version
8.7.5

"next": "^14.1.3"

NextJS (FE)

Call to getFeatureFlagEnvironmentContext took 620.0694000124931 milliseconds.
Call to getFeatureFlagEnvironmentContext took 672.3860999941826 milliseconds.
Call to getFeatureFlagEnvironmentContext took 756.7300000190735 milliseconds.
Call to getFeatureFlagEnvironmentContext took 764.7412999868393 milliseconds.
Call to getFeatureFlagEnvironmentContext took 793.1011999845505 milliseconds.
Call to getFeatureFlagEnvironmentContext took 10885.290700018406 milliseconds.  // 10 seconds
Call to getFeatureFlagEnvironmentContext took 11180.11109995842 milliseconds. // 11 seconds

NestJS (BE)

/**
* Note: the last 2 requests were received 10-11 seconds (3:19:31) after Next began rendering at 3:19:21 (!!!)*
*/
[Nest] 03/12/2024, 3:19:21 AM     LOG [EnvController] Call to getForEnvironment took 160.92500001192093 milliseconds.
[Nest] 03/12/2024, 3:19:21 AM     LOG [EnvController] Call to getForEnvironment took 175.3522999882698 milliseconds.
[Nest] 03/12/2024, 3:19:21 AM     LOG [EnvController] Call to getForEnvironment took 174.50929999351501 milliseconds.
[Nest] 03/12/2024, 3:19:21 AM     LOG [EnvController] Call to getForEnvironment took 177.85029995441437 milliseconds.
[Nest] 03/12/2024, 3:19:21 AM     LOG [EnvController] Call to getForEnvironment took 170.40650004148483 milliseconds.
[Nest] 03/12/2024, 3:19:31 AM     LOG [EnvController] Call to getForEnvironment took 173.81690001487732 milliseconds.
[Nest] 03/12/2024, 3:19:32 AM     LOG [EnvController] Call to getForEnvironment took 187.19900000095367 milliseconds.

waterfall

kuki-quupi commented 4 months ago

I'm having the same issue with Next.js v 14.1.4. I'm calling the same fetch with cache:"no-store" function getData in layout and then in child page of that layout. I expect the data to be memoized in first fetch (in layout) and on second call of the getData just to render the memoized data, but it always fetches the data twice (when layout is rendered and when page is rendered). This is happening in both build and dev mode.

kuki-quupi commented 4 months ago

I found out that memoization works in case when you have the same level components. For example, if i have an URL /dashboard and for that URL im using layout and page.tsx, and if i call the same fetch function in both of these then that fetch function is memoized. But, when i have URL /dashboard/user, and if i fetch the data in dashboard layout and page and then again the same fetch function in /user page, then data is not memoized and i get two fetch calls. Would like to hear others experience...

psikoi commented 4 months ago

I found out that memoization works in case when you have the same level components. For example, if i have an URL /dashboard and for that URL im using layout and page.tsx, and if i call the same fetch function in both of these then that fetch function is memoized. But, when i have URL /dashboard/user, and if i fetch the data in dashboard layout and page and then again the same fetch function in /user page, then data is not memoized and i get two fetch calls. Would like to hear others experience...

I think you expectation was misaligned with what this memoization is.

For the data to be "memoized" across two different page requests, it would need to be cached. The description of this utility says:

unstable_cache allows you to cache the results of expensive operations, like database queries, and reuse them across multiple requests.

However, the type of "deduping" or "memoization" discussed in this thread only exists for the duration of a single request, meaning if for the same page load you were fetching the same data 3 times (once in layout, once in page and maybe once in generateMetadata), this memoization would make sure this fetch only happened once.

This can be done using React's cache function. The result of this computation is memoized for the duration of the request, but it is not stored and re-used for other pages (read the "Caveats" section on react docs I linked).

I understand this can be confusing as both functions have the same name, but they have different purposes, I like to thing of React's cache function as memoization, rather than an actual cache.

Hopefully this can help others, and hopefully I didn't get the details wrong here, I am also learning this as I go.

kuki-quupi commented 4 months ago

@psikoi

Well it might be that docs are not correct. Because it has sense that data is being memoized only during the rendering process of the single page. But docs are telling us that fetch function should be memoized through multiple layouts and pages, which would mean that data would be memoized on different URLs...

Check out the diagram from the docs. It is showing 2 layers of depth, so 2 different pages should be rendered, and still fetch request should be memoized.

Screenshot from 2024-03-29 15-21-08

It is either they should fix the fetch memoization or align docs with the true nature of fetch memoization.

mikechabotcandy commented 4 months ago

The docs say requests are memoized only when the URL and options are identical.

lorenzosim commented 4 months ago

Yes, I think the docs are very clear, it's not meant to de-dup across different URLs.

The problem is that it doesn't actually work. Posting another repo with 14.1.4, the problem is still there: https://codesandbox.io/p/devbox/next-double-fetch-mcfppg

FrenchMajesty commented 3 months ago

I am 6 days into the NextJS and it seems like a mess to me. How are people using this in production?

The framework tries to do too much optimizations on behalf of the developer and it leads to their system incurring bugs (naturally) which then are addressed with great delay and difficulty due to the massive breath of capabilities vs. limited developers to tackle issues.

At my company we are definitively pulling out as soon as we can. At least I can trust Express to work as expected and React SSR individually. NextJS on the other end? Not so confident

rnnyrk commented 3 weeks ago

I'm using the storyblok-js-client to fetch data for the pages. I'm trying to using React cache as mentioned over here, but the dedupe doesn't seem to work resulting in a lot of requests.

How can I solve this?

Current implementation:

export async function generateMetadata({
  params: { locale, site, slug },
}: GeneralSlugPageProps) {
  const config = await getSiteConfig({ site, locale });
  const page = await getBySlug({
    locale,
    site,
    slug: slug ? slug.join("/") : "/",
  });

  return siteMetadata({ ... });
}

export default async function GeneralSlugPage({
  params: { site, locale, slug },
}: GeneralSlugPageProps) {
  const { header, footer } = await getLayout({
    site,
    locale,
  });

  const page = await getBySlug({
    locale,
    site,
    slug: slug ? slug.join("/") : "/",
  });

  return (..)
};

I've tried creating a separate cached function in this component and call getCachedPageBySlug instead of getBySlug in generateMetadata and GeneralSlugPage. But the result in amount of calls doesn't change.

export const getCachedPageBySlug = cache(
  async ({ locale, site, slug }) =>
    await getBySlug({
      locale,
      site,
      slug: `tickets/${slug.join("/")}`,
    })
);

Afterwards I reverted this change and moved the cache inside the e.g. getBySlug function. Like:

export const  getBySlug = cache(async<T = Record<string, any>>({
  site,
  slug,
  locale = "nl",
  params,
}: GetBySlugProps): Promise<StoryblokStory<T>> => {
  try {
    const story = await storyblokClient.getBySlug({
      site,
      slug: `page/${slug || ""}`,
      locale,
      params,
    });

    return story.data.story;
  } catch (error) {
    if (error.message === "Not Found") {
      return notFound();
    }

    throw error;
  }
};

But again the results do not change. How to dedupe calls correctly?

michalcizek commented 3 weeks ago

I'm using the storyblok-js-client to fetch data for the pages. I'm trying to using React cache as mentioned over here, but the dedupe doesn't seem to work resulting in a lot of requests.

How can I solve this?

Current implementation:

export async function generateMetadata({
  params: { locale, site, slug },
}: GeneralSlugPageProps) {
  const config = await getSiteConfig({ site, locale });
  const page = await getBySlug({
    locale,
    site,
    slug: slug ? slug.join("/") : "/",
  });

  return siteMetadata({ ... });
}

export default async function GeneralSlugPage({
  params: { site, locale, slug },
}: GeneralSlugPageProps) {
  const { header, footer } = await getLayout({
    site,
    locale,
  });

  const page = await getBySlug({
    locale,
    site,
    slug: slug ? slug.join("/") : "/",
  });

  return (..)
};

I've tried creating a separate cached function in this component and call getCachedPageBySlug instead of getBySlug in generateMetadata and GeneralSlugPage. But the result in amount of calls doesn't change.

export const getCachedPageBySlug = cache(
  async ({ locale, site, slug }) =>
    await getBySlug({
      locale,
      site,
      slug: `tickets/${slug.join("/")}`,
    })
);

Afterwards I reverted this change and moved the cache inside the e.g. getBySlug function. Like:

export const  getBySlug = cache(async<T = Record<string, any>>({
  site,
  slug,
  locale = "nl",
  params,
}: GetBySlugProps): Promise<StoryblokStory<T>> => {
  try {
    const story = await storyblokClient.getBySlug({
      site,
      slug: `page/${slug || ""}`,
      locale,
      params,
    });

    return story.data.story;
  } catch (error) {
    if (error.message === "Not Found") {
      return notFound();
    }

    throw error;
  }
};

But again the results do not change. How to dedupe calls correctly?

@rnnyrk You are passing a new object to the cached function getBySlug every time. The arguments to the function act as a cache key and therefore you make two separate calls. If you want to have this cached, you have to make sure that the arguments have referential equality.

So you could for example simplify the function so that the arguments are only primitives (like the slug and the locale)

const getBySlug = cache(async (slug: string, locale: string, ...) => {...})

Or you could maybe stringify the input object, pass it as a string to the cached function and parse it inside, but that seems like a dirty hack. Not sure if there are other ways to guarantee referential equality between the page and the generate metadata function in your case when the input values are dynamic. If anyone knows, I would be interested too.

rnnyrk commented 1 week ago

@michalcizek Thanks, the solution in the end was indeed to use params directly instead of passing an object to cache functions.