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(service-worker): continue serving api requests on cache failure #32996
fix(service-worker): continue serving api requests on cache failure #32996
Conversation
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.
Thx, @apocalyp0sys! This goes into the right direction. (Also, love the tests 😍)
I have left a couple of suggestions. In addition, could you split up the PR into two commits: a refactor
one to add the lru
parameter to safeCacheResponse
(since that is actually unrelated to the fix) and a fix
one to actually fix the bug?
Make safe caching and unsafe caching methods compatible so they can be swapped. Gives more flexibility when writing http response processing code.
b47ae81
to
1f78c57
Compare
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.
Awesome! Thx for making the changes, @apocalyp0sys 👍
I left a couple more minor comments, but otherwise lgtm 🎉
1f78c57
to
dbcb09c
Compare
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.
Yay! Almost there 😅
When responses are cached ok during sw initialization, but caching throws an error when handling api response, this response never gets to client. Fix response delivery by catching errors, add logging and 2 test cases. Fixes angular#21412
dbcb09c
to
df1e2df
Compare
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.
LGTM 🎉 Thx, @apocalyp0sys! ❤️
I was unable to merge this to the patch branch as it causes conflicts. Could we create a new PR for patch branch? |
Should i create a new PR targeting |
Yes, please, @apocalyp0sys 🙏 |
…le (angular#32996) Make safe caching and unsafe caching methods compatible so they can be swapped. Gives more flexibility when writing http response processing code. PR Close angular#32996 (cherry picked from commit 1353afc)
When responses are cached ok during sw initialization, but caching throws an error when handling api response, this response never gets to client. Fix response delivery by catching errors, add logging and 2 test cases. Same changes for master branch in PR angular#32996 Fixes angular#21412
…le (angular#32996) Make safe caching and unsafe caching methods compatible so they can be swapped. Gives more flexibility when writing http response processing code. PR Close angular#32996
…ngular#32996) When responses are cached ok during sw initialization, but caching throws an error when handling api response, this response never gets to client. Fix response delivery by catching errors, add logging and 2 test cases. Fixes angular#21412 PR Close angular#32996
…le (angular#32996) Make safe caching and unsafe caching methods compatible so they can be swapped. Gives more flexibility when writing http response processing code. PR Close angular#32996
…ngular#32996) When responses are cached ok during sw initialization, but caching throws an error when handling api response, this response never gets to client. Fix response delivery by catching errors, add logging and 2 test cases. Fixes angular#21412 PR Close angular#32996
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When responses are cached ok during sw initialization, but caching throws an error when handling api response, this response never gets to client.
Issue Number: #21412
What is the new behavior?
Fix response delivery by catching errors and add 2 test cases.
Does this PR introduce a breaking change?