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

fix(service-worker): correctly handle failed cache-busted request #39786

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Nov 20, 2020

Since 5be4edf, a failing cache-busted network request (such as requests for fetching an uncached asset) will cause the ServiceWorker to incorrectly enter a degraded EXISTING_CLIENTS_ONLY mode. A failing network request could be caused by many reasons, including the client or server being offline, and does not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in cacheBustedFetchFromNetwork() to correctly handle errors in network requests.
For more details on the problem and the implemented fix see #39775.

Fixes #39775.

@gkalpak gkalpak added type: bug/fix regression Indicates than the issue relates to something that worked in a previous version target: patch This PR is targeted for the next patch release area: service-worker Issues related to the @angular/service-worker package P2 The issue is important to a large percentage of users, with a workaround labels Nov 20, 2020
@ngbot ngbot bot modified the milestone: needsTriage Nov 20, 2020
@google-cla google-cla bot added the cla: yes label Nov 20, 2020
@ngbot ngbot bot modified the milestone: needsTriage Nov 20, 2020
@mary-poppins
Copy link

You can preview d834d24 at https://pr39786-d834d24.ngbuilds.io/.

@gkalpak gkalpak marked this pull request as ready for review November 20, 2020 17:57
if (networkResult.ok) {
let response = await this.safeFetch(req);

// Decide whether a cache-busted request is necessary. A cache-busted request is necessary iff
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Decide whether a cache-busted request is necessary. A cache-busted request is necessary iff
// Decide whether a cache-busted request is necessary. A cache-busted request is necessary if

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this was intended, but I'll change it since it might confuse some people 😅
Thx for taking a look ✨

@gkalpak gkalpak force-pushed the fix-sw-failing-network-requests branch from d834d24 to 31f8591 Compare November 23, 2020 09:58
@mary-poppins
Copy link

You can preview 31f8591 at https://pr39786-31f8591.ngbuilds.io/.

Since 5be4edf, a failing cache-busted
network request (such as requests for fetching uncached assets) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.
For more details on the problem and the implemented fix see angular#39775.

Fixes angular#39775
@gkalpak gkalpak force-pushed the fix-sw-failing-network-requests branch from 31f8591 to febee11 Compare November 23, 2020 18:15
@mary-poppins
Copy link

You can preview febee11 at https://pr39786-febee11.ngbuilds.io/.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM

@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Nov 23, 2020
AndrewKushnir pushed a commit that referenced this pull request Nov 23, 2020
…9786)

Since 5be4edf, a failing cache-busted
network request (such as requests for fetching uncached assets) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.
For more details on the problem and the implemented fix see #39775.

Fixes #39775

PR Close #39786
@gkalpak gkalpak deleted the fix-sw-failing-network-requests branch November 24, 2020 16:59
@angular-automatic-lock-bot
Copy link

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 Jan 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(service-worker) Regression with failed cache-busted request.
4 participants