Skip to content

feat(service-worker): add option to config when to register sw #21842

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

feat(service-worker): add option to config when to register sw #21842

wants to merge 8 commits into from
+534 −45

Conversation

JiaLiPassion
Copy link
Contributor

@JiaLiPassion JiaLiPassion commented Jan 28, 2018 •
edited by gkalpak
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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: #20970
Some times, angular will start up some long macroTask such as a long delay setTimeout (in #20970, firebase auth will start a 200s timeout to do token refresh). so ngZone will not become stable and will not trigger service-worker to do registration.

What is the new behavior?

I think there are so many cases that when startup, there will be macroTask not complete or cost a lot of time, so in this PR, my idea is add a callback to let user to be able to choose when to register.

I added a property in RegistrationOptions, the sample is here.
DevIntent/angular-example#1

@NgModule({
...
imports: [
ServiceWorkerModule.register('/ngsw-worker.js', {enabled: true,
       toInitFactory: AppComponent.getServiceWorkerInitFactory})]
...

and getServiceWorkerInitFactory will return a promise, user can resolve it by their choice.
the sample code is here,

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

if the idea is ok, I will update the document.

This PR incorporates #26002.

Sorry, something went wrong.

@JiaLiPassion JiaLiPassion mentioned this pull request Jan 28, 2018
Closed
@Splaktar
Copy link
Contributor

Splaktar commented Jan 28, 2018 •
edited
Loading

Looks like the build failed with:
"Error: Public API differs from golden file. Please run gulp public-api:update."

@Splaktar
Copy link
Contributor

Would going down a path similar to how preloading lazy loaded routes works be better?

I.e. define a swRegistrationStrategy parameter that then has a couple of Angular provided defaults (like registerImmediately and registerWhenStable). Then allow it to also take user defined strategies similar to how SelectivePreloadingStrategy implements a custom strategy based on PreloadingStrategy.

@JiaLiPassion
Copy link
Contributor Author

@Splaktar , thank you for review, I will update the PR and let you review again.

@JiaLiPassion
Copy link
Contributor Author

@Splaktar , I have updated the PR, change the option to

  • registrationStrategy: optional parameter, specify the strategy that when to register service worker, the available options are:
    • registerWhenStable: this is the default behavior, service worker will register when Application is stable (no microTasks or macroTasks remain).
    • registerImmediately: register immediately without waiting application to become stable.
    • registerDelay:timeout : register after timeout, timeout is a number to specify the delay timeout, for example registerDelay:5000 means register after 5 seconds.
    • a factory to get Observable : you can also specify a factory which return an Observable, service worker will register when the Observable emit for the first time.

I also update the sample at DevIntent/angular-example#1, please review.

@jasonaden jasonaden added the area: service-worker Issues related to the @angular/service-worker package label Jan 29, 2018
Splaktar
Splaktar reviewed Jan 30, 2018
View reviewed changes
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

I like this direction that this is heading 😄

packages/service-worker/src/module.ts Outdated
navigator.serviceWorker.register(script, {scope: options.scope});
} else if (registrationStrategy.indexOf('registerDelay') !== -1) {
const split = registrationStrategy.split(':');
const delay = split.length > 1 ? split[1] : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the default delay be non-zero? Perhaps 5 or 10s? It would be better to give an intelligent default rather than falling back to just be the same as registerImmediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the zero fallback is a little different with registerImmediately.

  • registerImmediately means no delay at all.
  • zero means setTimeout(register, 0), it will still wait all microTasks to finish.

So I think maybe zero is acceptable. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, can you please specify those details about this default delay in the docs?

aio/content/guide/service-worker-getting-started.md Outdated
@@ -66,6 +66,31 @@ Add `ServiceWorkerModule` to the `@NgModule` `imports` array. Use the `register(

<code-example path="service-worker-getting-started/src/app/app.module.ts" linenums="false" title="src/app/app.module.ts" region="sw-module"> </code-example>

You can pass some options to `register()` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can pass some options to the register() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I will remove the ` mark.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, the ` is fine, I just added a "the".

aio/content/guide/service-worker-getting-started.md Outdated
return someService.initData(); // return an Observable, when data is available, we can register service worker.
)};
</code-pane>
</code-tabs>
Copy link
Contributor

Choose a reason for hiding this comment

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

This formatting doesn't seem to be rendering properly in GitHub Markdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The <code-tabs> and <code-pane> will be parsed by dgeni, I have tested locally, this part can be rendered correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that dgeni will code parse comments for addition to docs, but it also parses markdown?

aio/content/guide/service-worker-getting-started.md Outdated
You can pass some options to `register()` method.
- enabled: optional parameter, by default is true, if enabled is false, the module will behave like the browser not support service worker, and service worker will not be registered.
- scope: optional parameter, to specify the subset of your content that you want the service worker to control.
- registrationStrategy: optional parameter, specify the strategy that when to register service worker, the available options are:
Copy link
Contributor

Choose a reason for hiding this comment

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

specify a strategy that determines when to register the service worker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

aio/content/guide/service-worker-getting-started.md Outdated
- enabled: optional parameter, by default is true, if enabled is false, the module will behave like the browser not support service worker, and service worker will not be registered.
- scope: optional parameter, to specify the subset of your content that you want the service worker to control.
- registrationStrategy: optional parameter, specify the strategy that when to register service worker, the available options are:
- registerWhenStable: this is the default behavior, service worker will register when Application is stable (no microTasks or macroTasks remain).
Copy link
Contributor

Choose a reason for hiding this comment

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

the service worker will register when the application is stable (no microTasks or macroTasks remain).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, change Application to application.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added two "the"s in there too

aio/content/guide/service-worker-getting-started.md Outdated
- scope: optional parameter, to specify the subset of your content that you want the service worker to control.
- registrationStrategy: optional parameter, specify the strategy that when to register service worker, the available options are:
- registerWhenStable: this is the default behavior, service worker will register when Application is stable (no microTasks or macroTasks remain).
- registerImmediately: register immediately without waiting application to become stable.
Copy link
Contributor

Choose a reason for hiding this comment

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

without waiting for the application to become stable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, add the before application.

aio/content/guide/service-worker-getting-started.md Outdated
- registrationStrategy: optional parameter, specify the strategy that when to register service worker, the available options are:
- registerWhenStable: this is the default behavior, service worker will register when Application is stable (no microTasks or macroTasks remain).
- registerImmediately: register immediately without waiting application to become stable.
- registerDelay:timeout : register after timeout, timeout is a number to specify the delay timeout, for example `registerDelay:5000` means register after 5 seconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

registerDelay:timeout: register after the timeout period. timeout is the number of milliseconds to delay registration. For example: registerDelay:5000 would register the service worker after 5 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

aio/content/guide/service-worker-getting-started.md Outdated
- registerWhenStable: this is the default behavior, service worker will register when Application is stable (no microTasks or macroTasks remain).
- registerImmediately: register immediately without waiting application to become stable.
- registerDelay:timeout : register after timeout, timeout is a number to specify the delay timeout, for example `registerDelay:5000` means register after 5 seconds.
- a factory to get Observable : you can also specify a factory which return an Observable, service worker will register when the Observable emit for the first time.
Copy link
Contributor

Choose a reason for hiding this comment

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

A factory to get an Observable: You can also specify a factory which returns an Observable. The service worker will be registered the first time that a value is emitted by the Observable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

@Splaktar
Copy link
Contributor

@alxhub we would love some feedback on this approach before more effort is spent on this.

@JiaLiPassion
Copy link
Contributor Author

@Splaktar , thank you for the detailed review, I have updated the PR, and about the markdown format, I just took a screenshot, please check it, thank you.
screeshot

@JiaLiPassion JiaLiPassion mentioned this pull request Feb 7, 2018
Closed
@ngbot
Copy link

ngbot bot commented Apr 17, 2018

Hi @JiaLiPassion! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment
@ngbot
Copy link

ngbot bot commented Apr 25, 2018

Hi @JiaLiPassion! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@gkalpak gkalpak mentioned this pull request Jul 27, 2018
Closed
@Splaktar
Copy link
Contributor

@JiaLiPassion can you please rebase this?

@JiaLiPassion
Copy link
Contributor Author

@Splaktar, sure, will do.

@Splaktar
Copy link
Contributor

It looks like the last commit broke the build due to TypeScript 3.1.x changes.

@JiaLiPassion
Copy link
Contributor Author

Got it, I will fix it today.

@JiaLiPassion
Copy link
Contributor Author

@Splaktar, I have updated the source, please review , thanks.

@Splaktar
Copy link
Contributor

LGTM! CI Tests now pass. Thank you.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 7, 2018
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 25, 2019
@ngbot
Copy link

ngbot bot commented Apr 25, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "cla/google" is failing
    pending missing required labels: cla: yes
    pending forbidden labels detected: cla: no
    pending status "ci/circleci: setup" is pending
    pending missing required status "ci/circleci: build"
    pending missing required status "ci/circleci: lint"
    pending missing required status "ci/circleci: publish_snapshot"
    pending missing required status "ci/angular: size"

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
Copy link
Member

gkalpak commented Apr 25, 2019 •
edited
Loading

  • merge-assistance: Requires manual CLA verification.
  • merge-assistance: The missing requested reviews (see below) are not necessary (were caused by an earlier bad rebase) and even if they were they would be covered by @IgorMinar's global approval:
    • fw-compiler
    • fw-core
    • fw-docs-observables
    • fw-forms
    • fw-i18n
    • fw-router
    • fw-server
    • fw-testing
    • tools-language-service

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
@AndrewKushnir
feat(service-worker): expose SwRegistrationOptions token to allow r…
39c0152 
…untime config (#21842)

Previously, the ServiceWorker registration options should be defined as
an object literal (in order for them to be compatible with Ahead-of-Time
compilation), thus making it impossible to base the ServiceWorker
behavior on runtime conditions.
This commit allows specifying the registration options using a regular
provider, which means that it can take advantage of the `useFactory`
option to determine the config at runtime, while still remaining
compatible with AoT compilation.

PR Close #21842
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
@gkalpak @AndrewKushnir
test(service-worker): expand SwRegistrationOptions tests and move t…
be28a6a 
…o separate file (#21842)

PR Close #21842
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
@gkalpak @AndrewKushnir
docs(service-worker): improve SwRegistrationOptions docs and add ex…
aa53d6c 
…ample (#21842)

PR Close #21842
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
@JiaLiPassion @AndrewKushnir
feat(service-worker): allow configuring when the SW is registered (#2…
4cfba58 
…1842)

Fixes #20970

PR Close #21842
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
@gkalpak @AndrewKushnir
refactor(service-worker): DRY up SW registration logic (#21842)
8f120af 
PR Close #21842
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
@gkalpak @AndrewKushnir
test(service-worker): add tests for `RegistrationOptions#registration…
957f594 
…Strategy` (#21842)

PR Close #21842
@jraadt jraadt mentioned this pull request May 1, 2019
Open
9 tasks
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@gkalpak @BioPhoton
refactor(service-worker): rename RegistrationOptions to `SwRegistra…
3415cfa 
…tionOptions` (angular#21842)

This is in preparation of making `RegistrationOptions` part of the
public API (in a subsequent commit).

PR Close angular#21842
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@BioPhoton
feat(service-worker): expose SwRegistrationOptions token to allow r…
106aa39 
…untime config (angular#21842)

Previously, the ServiceWorker registration options should be defined as
an object literal (in order for them to be compatible with Ahead-of-Time
compilation), thus making it impossible to base the ServiceWorker
behavior on runtime conditions.
This commit allows specifying the registration options using a regular
provider, which means that it can take advantage of the `useFactory`
option to determine the config at runtime, while still remaining
compatible with AoT compilation.

PR Close angular#21842
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@gkalpak @BioPhoton
test(service-worker): expand SwRegistrationOptions tests and move t…
9852599 
…o separate file (angular#21842)

PR Close angular#21842
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@gkalpak @BioPhoton
docs(service-worker): improve SwRegistrationOptions docs and add ex…
aa955f2 
…ample (angular#21842)

PR Close angular#21842
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@JiaLiPassion @BioPhoton
feat(service-worker): allow configuring when the SW is registered (an…
77aef6c 
…gular#21842)

Fixes angular#20970

PR Close angular#21842
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@gkalpak @BioPhoton
refactor(service-worker): DRY up SW registration logic (angular#21842)
53e4edd 
PR Close angular#21842
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@gkalpak @BioPhoton
test(service-worker): add tests for `RegistrationOptions#registration…
8be0a84 
…Strategy` (angular#21842)

PR Close angular#21842
Splaktar added a commit to DevIntent/angular-example that referenced this pull request Jun 22, 2019
@Splaktar
fix(sw): use registrationStrategy: 'registerWithDelay:5000'
954e461 
Relates to #1. Relates to angular/angular#21842.
@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 Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IgorMinar IgorMinar

@Splaktar Splaktar

@gkalpak gkalpak

Assignees

@IgorMinar IgorMinar

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 effort2: days feature Issue that requests a new feature merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: medium target: major This PR is targeted for the next major release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@JiaLiPassion @Splaktar @googlebot @gkalpak @IgorMinar @jasonaden @benlesh