Skip to content

fix(service-worker): allow creating post api requests after cache failure#33930

Closed
apocalyp0sys wants to merge 2 commits into
angular:masterfrom
apocalyp0sys:sw-mutating-requests-lru-cache-write-fix
Closed

fix(service-worker): allow creating post api requests after cache failure#33930
apocalyp0sys wants to merge 2 commits into
angular:masterfrom
apocalyp0sys:sw-mutating-requests-lru-cache-write-fix

Conversation

@apocalyp0sys
Copy link
Copy Markdown
Contributor

@apocalyp0sys apocalyp0sys commented Nov 20, 2019 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Before creating a mutating http request, service-worker invalidates lru cache entry and writes to cache storage. Therefore, cache storage failure can prevent making post requests.

Issue Number: #33793

What is the new behavior?

Cache storage error is catched and logged when trying to update lru cache table.

Does this PR introduce a breaking change?

  • Yes
  • No

@apocalyp0sys apocalyp0sys requested a review from a team November 20, 2019 12:18
@gkalpak gkalpak added area: service-worker Issues related to the @angular/service-worker package action: review The PR is still awaiting reviews from at least one requested reviewer PR target: TBD type: bug/fix labels Nov 20, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Nov 20, 2019
gkalpak
gkalpak approved these changes Nov 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@gkalpak gkalpak left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, @apocalyp0sys ❤️
(Nice commit message, btw 👍)

Comment thread packages/service-worker/worker/src/data.ts Outdated
@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 21, 2019
@apocalyp0sys apocalyp0sys force-pushed the sw-mutating-requests-lru-cache-write-fix branch 2 times, most recently from a8820d8 to 528477b Compare November 21, 2019 11:10
@gkalpak gkalpak added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker and removed PR target: TBD action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 21, 2019
@ngbot
Copy link
Copy Markdown

ngbot Bot commented Nov 21, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: integration_test" is failing
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@gkalpak gkalpak added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Nov 21, 2019
@gkalpak

This comment has been minimized.

Sign in to view
@matsko
Copy link
Copy Markdown
Contributor

matsko commented Nov 21, 2019

@apocalyp0sys please update the payload size amount so we can merge this.

@apocalyp0sys
refactor(service-worker): set http method in MockRequest constructor
72a2ea3 
Enable to create mocks for mutating http requests in tests
to check more scenarios.
@apocalyp0sys
fix(service-worker): allow creating post api requests after cache fai…
809e9da 
…lure

Before creating a mutating http request, service-worker
invalidates lru cache entry and writes to cache storage.
Therefore, cache storage failure can prevent making post requests.
Fix this by catching and logging cache error, add a test case.

Fixes angular#33793
@apocalyp0sys apocalyp0sys force-pushed the sw-mutating-requests-lru-cache-write-fix branch from 528477b to 809e9da Compare November 22, 2019 06:44
@apocalyp0sys
Copy link
Copy Markdown
Contributor Author

apocalyp0sys commented Nov 22, 2019

@matsko rebased onto master

@gkalpak gkalpak removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Nov 22, 2019
matsko pushed a commit that referenced this pull request Nov 22, 2019
@apocalyp0sys @matsko
refactor(service-worker): set http method in MockRequest constructor (#…
aa4d2b7 
…33930)

Enable to create mocks for mutating http requests in tests
to check more scenarios.

PR Close #33930
matsko pushed a commit that referenced this pull request Nov 22, 2019
@apocalyp0sys @matsko
fix(service-worker): allow creating post api requests after cache fai…
63c9123 
…lure (#33930)

Before creating a mutating http request, service-worker
invalidates lru cache entry and writes to cache storage.
Therefore, cache storage failure can prevent making post requests.
Fix this by catching and logging cache error, add a test case.

Fixes #33793

PR Close #33930
@matsko matsko closed this in 82be621 Nov 22, 2019
matsko pushed a commit that referenced this pull request Nov 22, 2019
@apocalyp0sys @matsko
fix(service-worker): allow creating post api requests after cache fai…
621b659 
…lure (#33930)

Before creating a mutating http request, service-worker
invalidates lru cache entry and writes to cache storage.
Therefore, cache storage failure can prevent making post requests.
Fix this by catching and logging cache error, add a test case.

Fixes #33793

PR Close #33930
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Dec 23, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Dec 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@gkalpak gkalpak gkalpak approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: service-worker Issues related to the @angular/service-worker package cla: yes target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

4 participants

@apocalyp0sys @gkalpak @matsko @googlebot