Skip to content

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

Closed
JiaLiPassion wants to merge 8 commits intoangular:masterfrom
JiaLiPassion:sw-init
Closed

feat(service-worker): add option to config when to register sw#21842
JiaLiPassion wants to merge 8 commits intoangular:masterfrom
JiaLiPassion:sw-init

Conversation

@JiaLiPassion
Copy link
Copy Markdown
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.

@JiaLiPassion JiaLiPassion mentioned this pull request Jan 28, 2018
Closed
@Splaktar
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Splaktar commented Jan 28, 2018

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
Copy Markdown
Contributor Author

JiaLiPassion commented Jan 29, 2018

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

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Jan 29, 2018

@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
Copy Markdown
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
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Jan 30, 2018

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
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Jan 30, 2018

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
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Jan 30, 2018

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
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Jan 30, 2018

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
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Jan 30, 2018

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
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Jan 30, 2018

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
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Jan 30, 2018

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
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Jan 30, 2018

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
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Jan 30, 2018

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
Copy Markdown
Contributor

Splaktar commented Jan 30, 2018

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

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Jan 30, 2018

@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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
Contributor

Splaktar commented Oct 18, 2018

@JiaLiPassion can you please rebase this?

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Oct 18, 2018

@Splaktar, sure, will do.

@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Oct 19, 2018

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

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Oct 19, 2018

Got it, I will fix it today.

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Oct 19, 2018

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

@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Oct 19, 2018

LGTM! CI Tests now pass. Thank you.

@googlebot
Copy link
Copy Markdown

googlebot commented Nov 7, 2018

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 and others added 8 commits April 25, 2019 16:51
@gkalpak
refactor(service-worker): rename RegistrationOptions to `SwRegistra…
ef0435f 
…tionOptions`

This is in preparation of making `RegistrationOptions` part of the
public API (in a subsequent commit).
@gkalpak
feat(service-worker): expose SwRegistrationOptions token to allow r…
5874101 
…untime config

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.
@gkalpak
test(service-worker): expand SwRegistrationOptions tests and move t…
f6d1c63 
…o separate file
@gkalpak
docs(service-worker): improve SwRegistrationOptions docs and add ex…
ea877d5 
…ample
@JiaLiPassion @gkalpak
feat(service-worker): allow configuring when the SW is registered
acec547 
Fixes angular#20970
@gkalpak
fixup! feat(service-worker): allow configuring when the SW is registered
f142987
@gkalpak
refactor(service-worker): DRY up SW registration logic
c6a21cc
@gkalpak
test(service-worker): add tests for `RegistrationOptions#registration…
5920d00 
…Strategy`
@googlebot
Copy link
Copy Markdown

googlebot commented Apr 25, 2019

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.

ℹ️ Googlers: Go here for more info.

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

googlebot commented Apr 25, 2019

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
@jraadt jraadt mentioned this pull request May 1, 2019
Open
9 tasks
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 15, 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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@IgorMinar IgorMinar IgorMinar approved these changes

@Splaktar Splaktar Splaktar approved these changes

@gkalpak gkalpak gkalpak approved these changes

Reviewers whose approvals may not affect merge requirements

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 Label used to distinguish feature request from other issues 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.

7 participants

@JiaLiPassion @Splaktar @googlebot @gkalpak @IgorMinar @jasonaden @benlesh