Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cachedEventHandler() needs at least 2 calls after it is invalid #1948

Closed
MickL opened this issue Nov 23, 2023 · 5 comments
Closed

cachedEventHandler() needs at least 2 calls after it is invalid #1948

MickL opened this issue Nov 23, 2023 · 5 comments

Comments

@MickL
Copy link
Contributor

MickL commented Nov 23, 2023

Environment

Nuxt: 3.8.2
H3: 1.9.0
nitropack: 2.8.0
Server: Vercel
Storage: Vercel KV (Redis)

Reproduction

server/api/tester.ts

export default cachedEventHandler(
  async (event): Promise<any> => {
    return Date.now();
  },
  {
    maxAge: 5, // seconds
  },
);

nuxt.config.ts

export default defineNuxtConfig({
  devtools: { enabled: true },
  nitro: {
    storage: {
      cache: {
        driver: 'vercelKV',
      },
    },
  },
});

Describe the bug

Using cachedEventHandler() with Vercel KV (Redis) requires at least 2 calls after the cache is invalid until the function runs again:

  1. GET request -> Function runs + is cached
  2. GET request -> Returns cached response
  3. Wait until maxAge is expired
  4. GET request -> Returns cached response (should run the function instead)
  5. GET request -> Function runs + is cached

Additional context

In development Nitro is using the FS driver and I there it seems to work as it should.

Logs

No response

@pi0
Copy link
Member

pi0 commented Nov 23, 2023

Hi. I made a minimal sandbox to reproduce: https://stackblitz.com/edit/github-avewtb?file=routes%2Findex.ts

It is in fact an expected behavior. By default, we enable Stable-While-Revalidate strategy. The response will be invalidated in the background that's why you immediately get the old one until this time passes. (but notice the log that handler is immediately invoked) If you wish to get response after handler resolved, you can disable SWR using swr: false. I'm not sure if it is your desired behavior or not?


Also really strange if fs/memory behavior is different than Vercel KV. It is likely that vercel has multiple nodes but i need to investigate more. Have you checked vercel function logs if there is any specific error?

@MickL
Copy link
Contributor Author

MickL commented Nov 23, 2023

Thanks for the super fast response! Shouldnt swr be disabled by default to prevent unexpected results and confusion? When the cache is invalid, I expect my handler function to run and return the new result immediately. Otherwise the cache seems broken to me. E.g. thinking about a button on a website that says "update" wouldnt return an updated result, isntead it would return the same cached data as before unless it is pressed a second time.

Within the docs in all examples swr is always explicitly set to true which also gives the assumption its default is false .

I made a minimal reproduction repo:
https://github.com/MickL/nuxt-nitro-cache

Deployed to Vercel using Vercel KV:
https://nuxt-nitro-cache.vercel.app/api/tester

Indeed within development (using FS driver) it reruns the function immediately. Maybe because on dev everything is so fast? This would mean that on dev it is "wrong" while on Vercel it is correct.

@pi0
Copy link
Member

pi0 commented Nov 23, 2023

PR welcome to clarify the docs 👍🏼 Benefits of SWR behavior for non-blocking responses was important enough that we initially decided on this default. We can discuss later to change it for next major versions of nitro if enough merits.

Yep maybe dev experience is fast enough so you haven't notice issue :p

With disabled swr do you still have issues with caching behavior on vercel?


Unrelated: Since saw date-fns in your example, consider it has a poor bundling result (for worker targets) (#1889)

@MickL
Copy link
Contributor Author

MickL commented Nov 23, 2023

I would highly suggest swr default set to false as this creates unexpected behavior: The cache is invalid but still returned. This might be valid for some edge cases, but most of the times users probably dont want to receive an invalid cache response.

Updated the docs to make this more clear: #1949

@pi0
Copy link
Member

pi0 commented Nov 23, 2023

Lets move default discussion to #1950

Thanks for docs PR.

Do we need to keep this issue still open?

@MickL MickL closed this as completed Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants