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

Allow to bypass .send on a cache miss #63

Closed
cascandaliato opened this issue Nov 18, 2020 · 3 comments
Closed

Allow to bypass .send on a cache miss #63

cascandaliato opened this issue Nov 18, 2020 · 3 comments

Comments

@cascandaliato
Copy link

Next.js example ssr-caching is currently broken (vercel/next.js#16372, vercel/next.js#16725) because there is no way to have Next.js ssr-render the page content on a cache miss without it also writing and .end-ing the response. Previous versions of the example relied on app.renderToHTML which has been deprecated because it's an internal API (vercel/next.js#14737).

I suggest we add a flag to bypass the .send() in cacheable-response when a cache miss happens, for those cases where the framework takes care of the req/res lifecycle till the end. We can add it either as an option in cacheableResponse({}) or as a property of .get()'s return value.

@Kikobeats
Copy link
Owner

Hey, although some people used cacheable-response in the past with Next.js, just I warn about the fact cacheable-response is a general-purpose library rather than a specific Next.js solution.

Saying that I feel cacheable-response as general-purpose library should handle the response properly. If Next.js should skip sending function under determinate situations, then the library should provide the basics primitives for doing that.

I suggest we add a flag to bypass the .send() in cacheable-response when a cache miss happens

so correct if I am wrong (mainly because I'm not using Next.js with cacheable-response), just passing isHit to send will resolve the issue?

Can you promote this into a PR? 🙂

@cascandaliato
Copy link
Author

Sorry, I should have been more accurate in my proposal:

  • when we call Next.js to render the page (and pass the result back to the library), we cannot stop Next.js from .send/.end-ing the response on its own;
  • if we send a result back to the library (because we want the library to cache it), then the library tries to set headers, send the response, etc. and this makes the server complain because the request has already been answered by Next.js;
    setHeaders({
  • I suggest we add an option to make the library skip the .setHeaders/.end/.send parts when isHit === false.
    image

If this enhancement makes sense to you, I'll work on a PR.

@Kikobeats
Copy link
Owner

Kikobeats commented Feb 1, 2021

I think this is closely related to #64 (comment)

I want to make this library Next.js friendly, but keeping in mind this is not a Next.js specific solution.

I'm very welcome to accept a PR doing the library easy to connect if the current functionality keeps the same.

so feel free to open a PR 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants