-
Notifications
You must be signed in to change notification settings - Fork 8
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
fix(sw): return 200-299,304 responses immediately #383
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Verified that subsequent load is almost instant:
Verified that cache is invalidated for updated github items (description updates):
@whizzzkid @momack2 please review. Preview urls for the urls Molly gave in slack are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok can confirm it's a bit faster, thanks @SgtPooki
if (!cachedResponse) { | ||
return false | ||
} | ||
if (!cachedResponse.ok || cachedResponse.status === 304) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can merge the two if statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, i was just trying to keep them separate and explicit for clarity
fixes #380
This PR updates the caching strategy to prevent the duplicate call to the backend
if we have a cached call, and returns it immediately.
One thing to note, is that vercel-cached responses return HTTP 304 responses,
and
response.ok
didn't check for that. This PR also addresses this.So, a call to the backend vercel function, that calls github does NOT happen IFF:
A backend call to the backend vercel function, that calls github, WILL happen IFF:
The cached response's max-age is "too old"