feat(service-worker): change default SW strategy behavior #35870

sonukapoor · 2020-03-05T10:48:43Z

Previously, there was a chance that the SW will never register
when there is a long-running task or recurring timeout. This commit
fixes this, by first trying to wait for the app to stabilize, however,
if that does not happen, then the SW will register after 30 seconds.

Closes #34464

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #34464

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

sonukapoor · 2020-03-05T11:19:57Z

@alxhub / @gkalpak My local lint and tests are all passing - not sure why CI is complaining. Can you help with this?

mary-poppins · 2020-03-05T13:22:50Z

You can preview 21e583d at https://pr35870-21e583d.ngbuilds.io/.
You can preview 0749d35 at https://pr35870-0749d35.ngbuilds.io/.

gkalpak · 2020-03-05T13:26:32Z

Thx, @sonukapoor, for taking this on. Can you please remove the formatting changes? They make the PR hard to review and the formatter is unhappy (you are probably running the wrong commands locally if lint passes).

Also, how are you running the tests locally? The failures on CI seem expected.

sonukapoor · 2020-03-05T13:29:08Z

@gkalpak. I am running yarn lint and yarn test in the aio folder. When I try to run similar commands within root then I get all kind of strange errors from bazel

gkalpak

Here are some more comments from a preliminary high-level review.

packages/service-worker/src/module.ts

gkalpak · 2020-03-05T14:28:41Z

@gkalpak. I am running yarn lint and yarn test in the aio folder. When I try to run similar commands within root then I get all kind of strange errors from bazel

These changes are outside the aio/ directory, so you need to run commands at the root of the repo 😁
Can you open a separate issue with the errors you are getting from bazel?

sonukapoor · 2020-03-06T02:58:56Z

Can you open a separate issue with the errors you are getting from bazel?

@gkalpak, Please see here: #35891

mary-poppins · 2020-03-06T03:26:46Z

You can preview 5d6531f at https://pr35870-5d6531f.ngbuilds.io/.

mary-poppins · 2020-03-06T03:50:45Z

You can preview 4b8b57a at https://pr35870-4b8b57a.ngbuilds.io/.

packages/service-worker/src/module.ts

packages/service-worker/test/module_spec.ts

…stration strategy Previously, when using the `registerWhenStable` ServiceWorker registration strategy (which is also the default) Angular would wait indefinitely for the [app to stabilize][1], before registering the ServiceWorker script. This could lead to a situation where the ServiceWorker would never be registered when there was a long-running task (such as an interval or recurring timeout). Such tasks can often be started by a 3rd-party dependency (beyond the developer's control or even without them realizing). In addition, this situation is particularly hard to detect, because the ServiceWorker is typically not used during development and on production builds a previous ServiceWorker instance might be already active. This commit enhances the `registerWhenStable` registration strategy by adding support for an optional `<timeout>` argument, which guarantees that the ServiceWorker will be registered when the timeout expires, even if the app has not stabilized yet. For example, with `registerWhenStable:5000` the ServiceWorker will be registered as soon as the app stabilizes or after 5 seconds if the app has not stabilized by then. Related to angular#34464. [1]: https://angular.io/api/core/ApplicationRef#is-stable-examples

…SW registration strategy

…p never stabilizes Previously, when using the default ServiceWorker registration strategy Angular would wait indefinitely for the [app to stabilize][1], before registering the ServiceWorker script. This could lead to a situation where the ServiceWorker would never be registered when there was a long-running task (such as an interval or recurring timeout). Such tasks can often be started by a 3rd-party dependency (beyond the developer's control or even without them realizing). In addition, this situation is particularly hard to detect, because the ServiceWorker is typically not used during development and on production builds a previous ServiceWorker instance might be already active. This commit fixes this by changing the default registration strategy from `registerWhenStable` to `registerWhenStable:30000`, which will ensure that the ServiceWorker will be registered after 30s at the latest, even if the app has not stabilized by then. Fixes angular#34464

… the app never stabilizes

…g app stabilization Previously, some of the built-in ServiceWorker registration strategies, namely `registerWithDelay:<timeout>` and `registerWhenStable:<timeout>`, would register potentially long-running timeout, thus preventing the app from stabilizing before the timeouts expired. This was especially problematic for the `registerWhenStable:<timeout>` strategy, which waits for the app to stabilize, because the strategy itself would prevent the app from stabilizing and thus the ServiceWorker would always be registered after the timeout. This commit fixes this by subscribing to the registration strategy observable outside the Angular zone, thus not affecting the app's stabilization.

gkalpak · 2020-03-24T20:59:47Z

Rebased on master, updated commit messages and fixed up commits. Let's see what CI thinks 😃
@sonukapoor, please take a look and make sure I didn't miss anything 🙏

sonukapoor · 2020-03-24T21:06:48Z

Rebased on master, updated commit messages and fixed up commits. Let's see what CI thinks 😃
@sonukapoor, please take a look and make sure I didn't miss anything 🙏

Looks good to me. Thanks for the updates - very much appreciated.

mary-poppins · 2020-03-24T21:09:38Z

You can preview 5bf2fe2 at https://pr35870-5bf2fe2.ngbuilds.io/.

…ffecting app stabilization

gkalpak

Thx, @sonukapoor ✨

mary-poppins · 2020-03-24T21:35:33Z

You can preview 22bf4cf at https://pr35870-22bf4cf.ngbuilds.io/.

…p never stabilizes (#35870) Previously, when using the default ServiceWorker registration strategy Angular would wait indefinitely for the [app to stabilize][1], before registering the ServiceWorker script. This could lead to a situation where the ServiceWorker would never be registered when there was a long-running task (such as an interval or recurring timeout). Such tasks can often be started by a 3rd-party dependency (beyond the developer's control or even without them realizing). In addition, this situation is particularly hard to detect, because the ServiceWorker is typically not used during development and on production builds a previous ServiceWorker instance might be already active. This commit fixes this by changing the default registration strategy from `registerWhenStable` to `registerWhenStable:30000`, which will ensure that the ServiceWorker will be registered after 30s at the latest, even if the app has not stabilized by then. Fixes #34464 PR Close #35870

…g app stabilization (#35870) Previously, some of the built-in ServiceWorker registration strategies, namely `registerWithDelay:<timeout>` and `registerWhenStable:<timeout>`, would register potentially long-running timeout, thus preventing the app from stabilizing before the timeouts expired. This was especially problematic for the `registerWhenStable:<timeout>` strategy, which waits for the app to stabilize, because the strategy itself would prevent the app from stabilizing and thus the ServiceWorker would always be registered after the timeout. This commit fixes this by subscribing to the registration strategy observable outside the Angular zone, thus not affecting the app's stabilization. PR Close #35870

angular-automatic-lock-bot · 2020-04-27T16:05:52Z

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.}

googlebot added the cla: yes label Mar 5, 2020

pullapprove bot requested a review from alxhub March 5, 2020 10:48

sonukapoor force-pushed the sw-stable-with-delay branch from 21e583d to 0749d35 Compare March 5, 2020 12:19

gkalpak changed the title ~~feat(docs-infra): change default SW strategy behavior~~ feat(service-worker): change default SW strategy behavior Mar 5, 2020

gkalpak requested review from gkalpak and removed request for alxhub March 5, 2020 13:22

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 feature Issue that requests a new feature labels Mar 5, 2020

ngbot bot modified the milestone: needsTriage Mar 5, 2020

gkalpak added the aio: preview label Mar 5, 2020

gkalpak added the target: major This PR is targeted for the next major release label Mar 5, 2020

gkalpak reviewed Mar 5, 2020

View reviewed changes

sonukapoor force-pushed the sw-stable-with-delay branch from 0749d35 to 5d6531f Compare March 6, 2020 03:18

sonukapoor force-pushed the sw-stable-with-delay branch 2 times, most recently from 6be81b0 to 4b8b57a Compare March 6, 2020 03:42

sonukapoor requested a review from gkalpak March 6, 2020 12:12

gkalpak requested changes Mar 6, 2020

View reviewed changes

gkalpak added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 6, 2020

gkalpak force-pushed the sw-stable-with-delay branch from afcdf51 to 50115ca Compare March 24, 2020 20:52

This comment has been minimized.

Sign in to view

googlebot added cla: no and removed cla: yes labels Mar 24, 2020

gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Mar 24, 2020

This comment has been minimized.

Sign in to view

googlebot added cla: yes and removed cla: no labels Mar 24, 2020

sonukapoor and others added 5 commits March 24, 2020 22:54

fixup! feat(service-worker): support timeout in registerWhenStable …

8052046

…SW registration strategy

fixup! fix(service-worker): by default register the SW after 30s even…

9809916

… the app never stabilizes

gkalpak force-pushed the sw-stable-with-delay branch from 50115ca to 5bf2fe2 Compare March 24, 2020 20:55

fixup! fix(service-worker): prevent SW registration strategies from a…

22bf4cf

…ffecting app stabilization

gkalpak force-pushed the sw-stable-with-delay branch from 5bf2fe2 to 22bf4cf Compare March 24, 2020 21:25

gkalpak approved these changes Mar 24, 2020

View reviewed changes

gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 24, 2020

alxhub closed this in 00efacf Mar 27, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 27, 2020

feat(service-worker): change default SW strategy behavior #35870

feat(service-worker): change default SW strategy behavior #35870

Uh oh!

Conversation

sonukapoor commented Mar 5, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

sonukapoor commented Mar 5, 2020

Uh oh!

mary-poppins commented Mar 5, 2020

Uh oh!

gkalpak commented Mar 5, 2020

Uh oh!

sonukapoor commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gkalpak commented Mar 5, 2020

Uh oh!

sonukapoor commented Mar 6, 2020

Uh oh!

mary-poppins commented Mar 6, 2020

Uh oh!

mary-poppins commented Mar 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gkalpak commented Mar 24, 2020

Uh oh!

sonukapoor commented Mar 24, 2020

Uh oh!

mary-poppins commented Mar 24, 2020

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Mar 24, 2020

Uh oh!

angular-automatic-lock-bot bot commented Apr 27, 2020

Uh oh!

Uh oh!

sonukapoor commented Mar 5, 2020 •

edited

Loading