feat(aio): Convert AIO to use the new Service Worker 5.0.0. by alxhub · Pull Request #19795 · angular/angular

alxhub · 2017-10-18T21:39:51Z

AIO is currently using a beta version of @angular/service-worker.
Since that was implemented, the SW has been rewritten and released
as part of Angular 5.0.0. This commit updates AIO to use the latest
implementation, with an appropriate configuration file that caches
the various AIO assets in useful ways.

gkalpak

🎉 ✨ (Quite excited about the dynamic content caching 😄)
A couple of comments/questions:

You can now remove the --ignore-packages @angular/service-worker part from here.
The SwUpdatesService spec needs to be updated too 😁
Do we still need ngsw-manifest.json?

gkalpak · 2017-10-18T21:59:27Z

Nit: Update event --> Activation event

gkalpak · 2017-10-18T22:06:38Z

Will the ServiceWorker still be registered on production only?
(How does it know which environment it is in?)

The module is only added if environment.production is set.

Will that work fine if the SW support doesn't exist at runtime (e.g. ios)?

Yes, the module itself does feature detection to decide whether to register the SW.

gkalpak · 2017-10-18T22:12:20Z

Shouldn't this be https too?

gkalpak · 2017-10-18T22:19:14Z

How does the new ServiceWorker handle routing? (For reference this was our previous config.)

gkalpak · 2017-10-18T22:22:47Z

AFAICT, it is possible to follow the previous config more closely (if not exactly) wrt to what gets cached and when.
Have you intentionally deviated from it?

lacolaco · 2017-10-19T01:26:17Z

Just out of curiosity, couldn't you use assets of the CLI?

The SW script has to be registered at the root of the page's origin, it can't come from a subdirectory.

webmaxru · 2017-10-26T05:27:30Z

Do you mean installMode?

webmaxru · 2017-10-26T05:29:14Z

I'd move this block with fonts to a separate group, because it will not follow prefetch rule anyway

It's conceptually part of the site.

webmaxru · 2017-10-26T08:47:02Z

My IDE says we have to add import 'rxjs/add/operator/do';

Splaktar

This will be a really useful 'real world' example once it gets to production!

Splaktar · 2017-11-06T21:19:27Z

Need to rebase to master to pick up 6b74883 and then update this to 5.0.0. It probably also worth using the Angular CLI 1.6.0-beta.0 release that supports Angular Service Worker.

IgorMinar · 2017-11-22T00:25:46Z

@alxhub can you please resurrect this PR and get it into a mergable state? thanks!

alxhub · 2017-11-29T21:48:54Z

@IgorMinar PTAL.

IgorMinar

I'm a bit concerned about the impact on ergonomics, let's discuss how we can resolve that - I'm afraid that this will require an API change/addition unless you have other ideas. Otherwise this looks good after the nitpicks are resolved.

IgorMinar · 2017-11-30T18:54:27Z

can't you update to cli@next and use the new build integration?

Yes, I could. But I don't want to make more changes to AIO's build system than I need to.

FWIW, we should soon upgrade to the latest-ish cli in #18428. So, maybe after that is merged you can use the build integration.

FWIW, we are now on cli@1.6.3.

IgorMinar · 2017-11-30T18:55:20Z

odd.. why the change to polyfills size?

No idea - it was failing.

It couldn't be failing for a 2B diff 😕
(FWIW, I think the polyfills size has been 37070 for a while, but it wasn't updated in the limits, since it is a small changed - much lower than 1%).

IgorMinar · 2017-11-30T18:59:41Z

It's not fixed. Did you forget to push the change?

IgorMinar · 2017-11-30T19:19:09Z

Also, I'd expect the main size to decrease. Isn't that the case or is it less than 1% so the size check in CI doesn't fail?

I'm not sure what you mean. It's probably less than 1%, this doesn't change much code.

IgorMinar · 2017-11-30T19:20:39Z

this is a bit odd, because it means that now you have to conditionally or optionally inject the ngsw services which makes the application code not very ergonomic.

Good point. I will fix that.

IgorMinar · 2017-11-30T19:22:38Z

ouch. that's ugly. can we do better? either inject it optionally, or even better always register the providers and inject them, but if we are on browser without SW support or in Dev mode, then make the services inert.. or something like that. We should really rethink this, because the current ergonomics are bad. I believe that this is what Stephen was pointing out as well...

IgorMinar · 2017-11-30T19:23:53Z

Will that work fine if the SW support doesn't exist at runtime (e.g. ios)?

IgorMinar · 2017-11-30T19:24:35Z

please rename this to "app-shell"

IgorMinar · 2017-11-30T19:29:24Z

there are actually two docs content groups - the ones that are available offline, and those that are lazy downloaded.

The offline docs include all marketing pages and images under generated/marketing/ I believe - check what the current site is fetching: https://angular.io/ngsw-manifest.json

The the remaining docs and assets should be downloaded on as needed basis. Your proposal changes the update behavior (compared to current angular.io) - I'm fine experimenting with the prefetch mode. We'll see how well this user experience works.

As discussed, the docs group has been moved to prefetch.

gkalpak · 2017-11-30T23:25:38Z

It couldn't be failing for a 2B diff 😕
(FWIW, I think the polyfills size has been 37070 for a while, but it wasn't updated in the limits, since it is a small changed - much lower than 1%).

gkalpak · 2017-11-30T23:27:12Z

Inconsistent spacing inside {...}.
_{I don't like the whitespace either, but... ¯\_(ツ)_/¯}

Did you mean "Not fixed"? 😁

gkalpak · 2017-11-30T23:32:11Z

IIRC, we added this to improve TTI or something.
Is this now handled by the ServiceWorkerModule internally?

mary-poppins · 2018-08-24T13:26:14Z

You can preview 1d82ed3 at https://pr19795-1d82ed3.ngbuilds.io/.

mary-poppins · 2018-08-24T18:51:00Z

You can preview 1896fa8 at https://pr19795-1896fa8.ngbuilds.io/.

ngbot · 2018-08-25T10:25:09Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     missing required label: "cla: yes"
     forbidden label detected: cla: no
     status "cla/google" is failing
     missing required status "code-review/pullapprove"
     status "ci/circleci: integration_test" is pending

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 · 2018-08-25T10:26:17Z

Note to caretaker:
The integration_test CircleCI failure is "inherited" from master and is not related to this PR. (This PR only changes aio/ stuff and does not affect integration tests.

googlebot · 2018-08-27T17:55:12Z

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

`%%DEPLOYMENT_HOST%%` has been assumed to be the host prefix for sitemap URLs since bf29936, but afaict this was never the case.

AIO is currently using a beta version of @angular/service-worker. Since that was implemented, the SW has been rewritten and released as part of Angular 5.0.0. This commit updates AIO to use the latest implementation, with an appropriate configuration file that caches the various AIO assets in useful ways.

…0.0.

…vkit/build-angular to 0.7.0-rc.3

The is a bug in versions 0.6.8+ that breaks when trying to use Jasmine's mock clock. Related to angular/angular-cli#11626.

This allows URLs to be passed through to the server (where they are properly redirected), instead of serving `index.html` from the SW. Known issue: `/docs/` will be passed through to the server. `/docs` (without the trailing slash) will be correctly treated as a navigation URL and handled by the SW. We don't link to `/docs/` from within the app, but if there are external links to `/docs/` they will require a round-trip to the server and will not work in offline mode.

googlebot · 2018-08-27T18:27:04Z

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 · 2018-08-27T18:33:11Z

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

mary-poppins · 2018-08-27T18:37:29Z

You can preview 0832d58 at https://pr19795-0832d58.ngbuilds.io/.

angular-automatic-lock-bot · 2019-09-13T23:53:05Z

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 Oct 18, 2017

gkalpak reviewed Oct 18, 2017

View reviewed changes

gkalpak added comp: aio state: WIP target: major This PR is targeted for the next major release labels Oct 18, 2017

lacolaco reviewed Oct 19, 2017

View reviewed changes

webmaxru reviewed Oct 26, 2017

View reviewed changes

lacolaco mentioned this pull request Oct 26, 2017

bug(service-worker): / path is omitted by ngsw-config #19945

Closed

IgorMinar added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Nov 3, 2017

Splaktar reviewed Nov 6, 2017

View reviewed changes

IgorMinar added this to the 5.0.x milestone Nov 22, 2017

IgorMinar added the feature Label used to distinguish feature request from other issues label Nov 22, 2017

alxhub force-pushed the aio-new-sw branch 2 times, most recently from fb0cbb6 to 35707ab Compare November 29, 2017 16:16

alxhub added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Nov 29, 2017

alxhub force-pushed the aio-new-sw branch from 35707ab to 571a600 Compare November 29, 2017 16:50

alxhub requested a review from IgorMinar November 29, 2017 21:48

alxhub force-pushed the aio-new-sw branch from 571a600 to 3eb0266 Compare November 30, 2017 18:43

IgorMinar previously requested changes Nov 30, 2017

View reviewed changes

gkalpak reviewed Nov 30, 2017

View reviewed changes

IgorMinar removed this from the 5.0.x milestone Dec 1, 2017

alxhub force-pushed the aio-new-sw branch 3 times, most recently from 7e7d935 to 9d4d9d4 Compare December 5, 2017 17:27

gkalpak and others added 19 commits August 27, 2018 21:23

test(docs-infra): correctly extract sitemap URLs

5ddc010

`%%DEPLOYMENT_HOST%%` has been assumed to be the host prefix for sitemap URLs since bf29936, but afaict this was never the case.

fixup! feat(docs-infra): Convert AIO to use the new Service Worker 5.…

7e9e472

…0.0.

fixup! feat(docs-infra): Convert AIO to use the new Service Worker 5.…

b5291c2

…0.0.

refactor(docs-infra): remove unused images

8f18059

refactor(docs-infra): move concept icons to more appropriate location

6e2e11e

fix(docs-infra): correctly show icon for fetch error when offline

6ba6bc6

build(docs-infra): enable ServiceWorker in cli config

9f69609

build(docs-infra): remove the dependency on rxjs-compat

6922a63

build(docs-infra): remove unused dependencies

9ada37b

build(docs-infra): upgrade @angular/cli to 6.0.3

63eb0ad

build(docs-infra): upgrade @angular/* to 6.0.2 and rxjs to 6.1.0

515178b

build(docs-infra): upgrade @angular/material to 6.0.2

6e41f61

build(docs-infra): update size limits

8293cfe

build(docs-infra): upgrade @angular/cli to 6.1.0-rc.3 and @angular-de…

146de97

…vkit/build-angular to 0.7.0-rc.3

build(docs-infra): upgrade @angular/* to 6.1.0-rc.3 and rxjs to 6.2.2

3df2e77

build(docs-infra): update size limits

c36774f

build(docs-infra): downgrade @angular-devkit/build-angular to 0.6.7

323afdf

The is a bug in versions 0.6.8+ that breaks when trying to use Jasmine's mock clock. Related to angular/angular-cli#11626.

gkalpak mentioned this pull request Sep 5, 2018

[6.1.x] test(docs-infra): fix double-slash in URL of aio_monitoring test #25820

Closed

gkalpak mentioned this pull request Sep 18, 2018

[6.1.x] update angular.io to the new ServiceWorker #25997

Closed

Conversation

alxhub commented Oct 18, 2017

Uh oh!

gkalpak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Nov 22, 2017

Uh oh!

alxhub commented Nov 29, 2017

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

gkalpak left a comment •

edited

Loading

alxhub Nov 30, 2017 •

edited

Loading