GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

Conversation

@alxhub

@alxhub alxhub commented Oct 18, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji lacolaco, m0t0r, webmaxru, Splaktar, and lazarljubenovic reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 5 reactions
@googlebot googlebot added the cla: yes label Oct 18, 2017
gkalpak
gkalpak reviewed Oct 18, 2017
View reviewed changes

@gkalpak gkalpak left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🎉 ✨ (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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/sw-updates/sw-updates.service.ts Outdated

@gkalpak gkalpak Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit: Update event --> Activation event

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/main.ts Outdated

@gkalpak gkalpak Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@alxhub alxhub Nov 29, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@alxhub alxhub Nov 30, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lazarljubenovic reacted with hooray emoji
All reactions
  • 🎉 1 reaction
Comment thread aio/src/ngsw.json Outdated

@gkalpak gkalpak Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Shouldn't this be https too?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread aio/src/ngsw.json Outdated

@gkalpak gkalpak Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread aio/src/ngsw.json Outdated

@gkalpak gkalpak Oct 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gkalpak gkalpak added comp: aio state: WIP target: major This PR is targeted for the next major release labels Oct 18, 2017
lacolaco
lacolaco reviewed Oct 19, 2017
View reviewed changes
Comment thread aio/package.json Outdated

@lacolaco lacolaco Oct 19, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Dec 4, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
webmaxru
webmaxru reviewed Oct 26, 2017
View reviewed changes
Comment thread aio/src/ngsw.json Outdated

@webmaxru webmaxru Oct 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do you mean installMode?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@alxhub alxhub Dec 4, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
webmaxru
webmaxru reviewed Oct 26, 2017
View reviewed changes
Comment thread aio/src/ngsw.json Outdated

@webmaxru webmaxru Oct 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Dec 4, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's conceptually part of the site.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
webmaxru
webmaxru reviewed Oct 26, 2017
View reviewed changes
Comment thread aio/src/app/sw-updates/sw-updates.service.ts Outdated

@webmaxru webmaxru Oct 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Nov 29, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do(ne).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lacolaco lacolaco mentioned this pull request Oct 26, 2017
bug(service-worker): / path is omitted by ngsw-config #19945
Closed
@IgorMinar 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
Splaktar reviewed Nov 6, 2017
View reviewed changes

@Splaktar Splaktar left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/package.json Outdated

@Splaktar Splaktar Nov 6, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@IgorMinar IgorMinar added this to the 5.0.x milestone Nov 22, 2017
@IgorMinar

IgorMinar commented Nov 22, 2017

Copy link
Copy Markdown
Contributor

@alxhub can you please resurrect this PR and get it into a mergable state? thanks!
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@IgorMinar IgorMinar added the feature Label used to distinguish feature request from other issues label Nov 22, 2017
@alxhub
alxhub force-pushed the aio-new-sw branch 2 times, most recently from fb0cbb6 to 35707ab Compare November 29, 2017 16:16
@alxhub 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
alxhub force-pushed the aio-new-sw branch from 35707ab to 571a600 Compare November 29, 2017 16:50
@alxhub
alxhub requested a review from IgorMinar November 29, 2017 21:48
@alxhub

alxhub commented Nov 29, 2017

Copy link
Copy Markdown
Member Author

@IgorMinar PTAL.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@alxhub
alxhub force-pushed the aio-new-sw branch from 571a600 to 3eb0266 Compare November 30, 2017 18:43
IgorMinar
IgorMinar previously requested changes Nov 30, 2017
View reviewed changes

@IgorMinar IgorMinar left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/package.json Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@alxhub alxhub Nov 30, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gkalpak gkalpak Nov 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar and alxhub reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@gkalpak gkalpak Jan 12, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

FWIW, we are now on [email protected].

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/scripts/_payload-limits.json Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

odd.. why the change to polyfills size?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Nov 30, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No idea - it was failing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gkalpak gkalpak Nov 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/index.html Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

remove

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Dec 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@IgorMinar IgorMinar Dec 6, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/scripts/_payload-limits.json Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Nov 30, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/app.module.ts Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@alxhub alxhub Nov 30, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good point. I will fix that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/sw-updates/sw-updates.service.ts Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread aio/src/main.ts Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread aio/src/ngsw-config.json Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

please rename this to "app-shell"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Dec 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/ngsw-config.json Outdated

@IgorMinar IgorMinar Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Dec 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gkalpak
gkalpak reviewed Nov 30, 2017
View reviewed changes
Comment thread aio/scripts/_payload-limits.json Outdated

@gkalpak gkalpak Nov 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/app/app.module.ts Outdated

@gkalpak gkalpak Nov 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@alxhub alxhub Dec 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gkalpak gkalpak Dec 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Did you mean "Not fixed"? 😁

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/src/main.ts Outdated

@gkalpak gkalpak Nov 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@alxhub alxhub Dec 4, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Exactly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Splaktar reacted with thumbs up emoji gkalpak reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 1 reaction
@IgorMinar IgorMinar removed this from the 5.0.x milestone Dec 1, 2017
@alxhub
alxhub force-pushed the aio-new-sw branch 3 times, most recently from 7e7d935 to 9d4d9d4 Compare December 5, 2017 17:27
@mary-poppins

mary-poppins commented Aug 24, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mary-poppins

mary-poppins commented Aug 24, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ngbot

ngbot Bot commented Aug 25, 2018

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "cla: yes"
    failure forbidden label detected: cla: no
    failure status "cla/google" is failing
    failure missing required status "code-review/pullapprove"
    pending 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gkalpak

gkalpak commented Aug 25, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@googlebot

googlebot commented Aug 27, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak and others added 19 commits August 27, 2018 21:23
@gkalpak
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.
@alxhub @gkalpak
feat(docs-infra): Convert AIO to use the new Service Worker 5.0.0.
803d169 
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
fixup! feat(docs-infra): Convert AIO to use the new Service Worker 5.…
7e9e472 
…0.0.
@gkalpak
fixup! feat(docs-infra): Convert AIO to use the new Service Worker 5.…
b5291c2 
…0.0.
@gkalpak
refactor(docs-infra): remove unused images
8f18059
@gkalpak
refactor(docs-infra): move concept icons to more appropriate location
6e2e11e
@gkalpak
fix(docs-infra): correctly show icon for fetch error when offline
6ba6bc6
@gkalpak
build(docs-infra): enable ServiceWorker in cli config
9f69609
@gkalpak
build(docs-infra): remove the dependency on rxjs-compat
6922a63
@gkalpak
build(docs-infra): remove unused dependencies
9ada37b
@gkalpak
build(docs-infra): upgrade @angular/cli to 6.0.3
63eb0ad
@gkalpak
build(docs-infra): upgrade @angular/* to 6.0.2 and rxjs to 6.1.0
515178b
@gkalpak
build(docs-infra): upgrade @angular/material to 6.0.2
6e41f61
@gkalpak
build(docs-infra): update size limits
8293cfe
@gkalpak
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
@gkalpak
build(docs-infra): upgrade @angular/* to 6.1.0-rc.3 and rxjs to 6.2.2
3df2e77
@gkalpak
build(docs-infra): update size limits
c36774f
@gkalpak
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
fix(docs-infra): ignore server-side redirected URLs in the SW
0832d58 
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

googlebot commented Aug 27, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@googlebot

googlebot commented Aug 27, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mary-poppins

mary-poppins commented Aug 27, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gkalpak 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 gkalpak mentioned this pull request Sep 18, 2018
[6.1.x] update angular.io to the new ServiceWorker #25997
Closed
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 13, 2019

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more reviewers
@lacolaco lacolaco lacolaco left review comments
@webmaxru webmaxru webmaxru left review comments
@Splaktar Splaktar Splaktar left review comments
@gkalpak gkalpak gkalpak left review comments
@IgorMinar IgorMinar IgorMinar approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gkalpak gkalpak

Labels

action: merge The PR is ready for merge by the caretaker cla: yes 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 target: major This PR is targeted for the next major release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@alxhub @IgorMinar @mary-poppins @gkalpak @petebacondarwin @googlebot @lacolaco @webmaxru @Splaktar @matsko @ocombe
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.