vercel / next.js

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

Fetch request memoization not working when cookies function imported #59010

Open drekembe opened 9 months ago

drekembe commented 9 months ago

Link to the code that reproduces this issue

https://github.com/drekembe/nextjs-request-memoization-bug

To Reproduce

npm install --force npx nx serve dragonradar npx nx serve my-nest-app

Then go to localhost:6777. In the console of the nest app, you'll see that the endpoint gets called only once. Then, go to user.tsx and uncomment the cookies import. Refresh the page and in the console of the nest app, you'll see that the endpoint now gets called three times.

If you remove the import, the request memoization for fetch will still not work. You have to restart the next dev server for it to behave normally again.

Current vs. Expected behavior

Currently, the fetch function doesn't get memoized on a per-request basis if the cookies function is imported in the component that does the fetch. I'd expect it to remain memoized. If not, I'd expect an explanation of this behavior in the docs. I haven't been able to find anything regarding this behavior in this section: https://nextjs.org/docs/app/building-your-application/caching#request-memoization

Verify canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.1.0: Mon Oct  9 21:27:24 PDT 2023; root:xnu-10002.41.9~6/RELEASE_ARM64_T6000
Binaries:
  Node: 18.18.2
  npm: 9.8.1
  Yarn: 1.22.19
  pnpm: 7.25.1
Relevant Packages:
  next: 14.0.4-canary.18
  eslint-config-next: 13.4.1
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.2.2
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

App Router, Data fetching (gS(S)P, getInitialProps)

Additional context

I'm testing this locally. I wanted to try and test this by building it and running it locally, but when I build it, the page gets fully statically generated (calls the endpoint only once during generation, regardless of the cookie import) and then when serving it, doesn't call the endpoints at all. This is another behavior that I find strange, but it's unrelated to this issue I guess

addrummond commented 6 months ago

I also ran into this problem and independently wrote some code to repro it before finding this issue. Here is my repro:

https://github.com/Multiverse-io/nextjs-cache-issue-repro

I believe it is very similar. It runs on next@14.1.2-canary.3.

The Readme for my repo has some notes on the crucial bits of code within NextJS responsible for this behavior. In particular:

The comment above staticGenerationStore.revalidate = 0 increases my suspicions that there is some unintended behavior here. The readme for my repro also describes some instances where passing the cache: 'force-cache' option to fetch alters the behavior of the function, even though the docs suggest that this is the default value for the cache option.

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 21.2.0: Sun Nov 28 20:28:41 PST 2021; root:xnu-8019.61.5~1/RELEASE_ARM64_T6000
Binaries:
  Node: 21.6.2
  npm: 10.2.4
Relevant Packages:
  next: 14.1.2-canary.3
  react: 18.2.0
  react-dom: 18.2.0
Next.js Config:
  output: N/A
addrummond commented 6 months ago

Ah, I see now that this behavior is documented in the 'Good to know' box here:

So although I find this behavior very confusing, I guess it is not a bug. Arguably this behavior does render some other statements in the docs inaccurate, though, e.g. the comment "force-cache is the default and can be omitted" in the first code example on the same page.

arjendevos commented 3 months ago

Same problem here

Edit by maintainer bot: Comment was automatically minimized because it was considered unhelpful. (If you think this was by mistake, let us know). Please only comment if it adds context to the issue. If you want to express that you have the same problem, use the upvote 👍 on the issue description or subscribe to the issue for updates. Thanks!

kag359six commented 2 months ago

@addrummond late here, but the docs you are referring to pertain to the Data Cache (ie. no-store and force-cache), which does not affect request memoization. Here is a direct quote from the docs explaining the difference:

Differences between the Data Cache and Request Memoization

While both caching mechanisms help improve performance by re-using cached data, the Data Cache is persistent across incoming requests and deployments, whereas memoization only lasts the lifetime of a request.

With memoization, we reduce the number of duplicate requests in the same render pass that have to cross the network boundary from the rendering server to the Data Cache server (e.g. a CDN or Edge Network) or data source (e.g. a database or CMS). With the Data Cache, we reduce the number of requests made to our origin data source.

In my case, regardless of the cache option provided, react is not correctly memoizing requests made in a single render pass.

However, using the cache api around a custom helper function like this:

const getUser = cache((userId) => {
   const c = cookies()
   return fetch(...)
})

does deduplicate requests, even when cookies are used.

addrummond commented 2 months ago

@kag359six Thanks for the explanation. Yes, at the time I commented on this thread I was confused between the Data Cache and memoization. Apologies for adding some noise to the discussion.

In my case, regardless of the cache option provided, react is not correctly memoizing requests made in a single render pass.

Last time I looked into this I found that memoization worked in production NextJS builds but not in dev mode. Not sure if that could be relevant to your issue.

kag359six commented 2 months ago

@addrummond no worries, it took me hours to find that snippet.

Regarding production vs. dev builds, I can confirm memoization works across both (or maybe I should say, neither work as expected lol)

Worth mentioning this memoization behavior is a react feature, not next.js