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

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

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
8 participants
@JiaLiPassion
Copy link
Contributor

commented Jan 28, 2018

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.

@Splaktar

This comment has been minimized.

Copy link
Member

commented Jan 28, 2018

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

@Splaktar

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

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

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:sw-init branch from 391e663 to 158c350 Jan 29, 2018

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

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.

@Splaktar
Copy link
Member

left a comment

I like this direction that this is heading 😄

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;

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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.

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Jan 30, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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

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

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Jan 30, 2018

Author Contributor

Got it, I will remove the ` mark.

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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

return someService.initData(); // return an Observable, when data is available, we can register service worker.
)};
</code-pane>
</code-tabs>

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Jan 30, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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

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:

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

specify a strategy that determines when to register the service worker

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Jan 30, 2018

Author Contributor

got it.

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

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Jan 30, 2018

Author Contributor

got it, change Application to application.

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

Added two "the"s in there too

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

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

without waiting for the application to become stable

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Jan 30, 2018

Author Contributor

got it, add the before application.

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

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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.

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Jan 30, 2018

Author Contributor

got it.

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

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jan 30, 2018

Member

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.

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Jan 30, 2018

Author Contributor

got it.

@Splaktar

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

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

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:sw-init branch from 158c350 to 020f78e Jan 30, 2018

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

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

@ngbot

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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!

@Splaktar

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@JiaLiPassion can you please rebase this?

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

@Splaktar, sure, will do.

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:sw-init branch from 020f78e to ec1330b Oct 19, 2018

@Splaktar

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

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

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

Got it, I will fix it today.

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:sw-init branch from ec1330b to cea41a3 Oct 19, 2018

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

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

@Splaktar

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

LGTM! CI Tests now pass. Thank you.

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:sw-init branch from cea41a3 to 078daf6 Oct 19, 2018

@gkalpak gkalpak force-pushed the JiaLiPassion:sw-init branch from 078daf6 to c7b8167 Nov 7, 2018

@googlebot

This comment has been minimized.

Copy link

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 gkalpak force-pushed the JiaLiPassion:sw-init branch from b6c84a7 to 5920d00 Apr 25, 2019

@googlebot

This comment has been minimized.

Copy link

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.

@ngbot

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

  • 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

This comment has been minimized.

Copy link

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 added a commit that referenced this pull request Apr 25, 2019

feat(service-worker): expose `SwRegistrationOptions` token to allow r…
…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 added a commit that referenced this pull request Apr 25, 2019

AndrewKushnir added a commit that referenced this pull request Apr 25, 2019

@jraadt jraadt referenced this pull request May 1, 2019

Open

Angular apps are never stable when using OktaAuthService. #423

2 of 9 tasks complete

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

refactor(service-worker): rename `RegistrationOptions` to `SwRegistra…
…tionOptions` (angular#21842)

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

PR Close angular#21842

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

feat(service-worker): expose `SwRegistrationOptions` token to allow r…
…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 added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.