docs(aio): add service worker guide #20736

kapunahelewong · 2017-12-01T17:40:03Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Adds new Service Worker documentation and example.

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

This PR is for working out the problems in the original SW PR #20716.

mary-poppins · 2017-12-01T17:50:25Z

You can preview 0aa146c at https://pr20736-0aa146c.ngbuilds.io/.

mary-poppins · 2017-12-01T21:07:25Z

You can preview 6d1d3be at https://pr20736-6d1d3be.ngbuilds.io/.

mary-poppins · 2017-12-01T21:36:47Z

You can preview 5e207d0 at https://pr20736-5e207d0.ngbuilds.io/.

mary-poppins · 2017-12-04T00:40:19Z

You can preview aedadf6 at https://pr20736-aedadf6.ngbuilds.io/.

mary-poppins · 2017-12-04T00:58:43Z

You can preview 8efb6bc at https://pr20736-8efb6bc.ngbuilds.io/.

mary-poppins · 2017-12-04T16:30:17Z

You can preview e0d7fe3 at https://pr20736-e0d7fe3.ngbuilds.io/.

mary-poppins · 2017-12-04T16:55:37Z

You can preview 4522b3f at https://pr20736-4522b3f.ngbuilds.io/.

mary-poppins · 2017-12-04T17:50:37Z

You can preview 766a0c9 at https://pr20736-766a0c9.ngbuilds.io/.

mary-poppins · 2017-12-04T22:10:38Z

You can preview dc39778 at https://pr20736-dc39778.ngbuilds.io/.

mary-poppins · 2017-12-05T18:04:45Z

You can preview c79275b at https://pr20736-c79275b.ngbuilds.io/.

mary-poppins · 2017-12-05T20:44:02Z

You can preview f945fe9 at https://pr20736-f945fe9.ngbuilds.io/.

mary-poppins · 2017-12-05T20:45:16Z

You can preview 83fe3dc at https://pr20736-83fe3dc.ngbuilds.io/.

mary-poppins · 2017-12-05T23:43:48Z

You can preview 8dd050d at https://pr20736-8dd050d.ngbuilds.io/.

IgorMinar

lgtm for package.json and other config changes. I haven't reviewed the content, but I don't expect that I was supposed to.

gkalpak

I haven't looked at the actual content (*.md files), but here are some comments:

The offline-checkbox image has the Memory highlighted (although not the current tab), which is a little confusing. Can we have an image with the Network tab highlighted?

The welcome-msg-en.png image says "Welcome to app", but the actual app will show "Welcome to Service Workers". Same for the welcome-msg-fr.png.

The empty aio/content/examples/service-worker-getstart/src/app/app.component.css should be removed.

gkalpak · 2017-12-06T13:41:29Z

aio/content/navigation.json

+              "tooltip": "The ngsw-config.json configuration file controls service worker caching behavior."
+            }
+          ]
+        },


Should Service Workers be in Fundamentals (rather than Techniques)? (Just checking...)

Service Workers is under Fundamentals.

gkalpak · 2017-12-06T13:44:57Z

aio/content/navigation.json

+            {
+              "url": "guide/service-worker-devops",
+              "title": "Service Workers in Production",
+              "tooltip": "Information about running applications with service workers, including application update management, debugging, and killing applications."


This tooltip is a little too long. How about:

Running applications with service workers, managing application update, debugging, and killing applications.

gkalpak · 2017-12-06T13:46:06Z

aio/content/navigation.json

+            {
+              "url": "guide/service-worker-configref",
+              "title": "Reference: Configuration File",
+              "tooltip": "The ngsw-config.json configuration file controls service worker caching behavior."


Why not Configuring service worker caching behavior (to be more consistent with other tooltips)?

gkalpak · 2017-12-06T13:55:41Z

aio/content/guide/service-worker-getstart.md

+If you look at the Network tab, you can verify that the service worker is active.
+
+<figure>
+  <img src="generated/images/guide/service-worker/sw-active.png" alt="Requests are marked as from ServiceWorker">


This image is too wide and gets truncated at certain screen sizes. Make sure it is no wider than 660px.

gkalpak · 2017-12-06T16:32:21Z

aio/content/examples/service-worker-getstart/src/app/app.component.ts

+@Component({
+  selector: 'app-root',
+  templateUrl: './app.component.html',
+  styleUrls: ['./app.component.css']


If this is not needed, let's remove it.

gkalpak · 2017-12-06T16:38:08Z

aio/content/examples/service-worker-getstart/src/app/app.component.spec.ts

+  it(`should have as title 'app'`, async(() => {
+    const fixture = TestBed.createComponent(AppComponent);
+    const app = fixture.debugElement.componentInstance;
+    expect(app.title).toEqual('app');


Shouldn't it be Service Workers? Are these tests run at all (I don't think so). Do we need them (given that they are pretty basic and the assertions could/should be covered in the e2e tests)?

gkalpak · 2017-12-06T16:39:47Z

aio/content/examples/service-worker-getstart/e2e/app.e2e-spec.ts

+
+describe('sw-example App', () => {
+  let page: AppPage;
+  let logo = element(by.css('img'));


Since this is only used once, why not move it inside the respective test?

gkalpak · 2017-12-06T16:41:53Z

aio/content/examples/service-worker-getstart/e2e/app.e2e-spec.ts

+      });
+  });
+
+});


We should somehow include the services into the tests. Right now, the only ServiceWorker related thing that is tested is ServiceWorkerModule.register(...). We should include the services into the app (i.e. inject them somewhere) and ensure that they at least do not break.

gkalpak · 2017-12-06T16:42:48Z

aio/content/examples/service-worker-getstart/src/ngsw-config.json

+    "resources": {
+      "files": [
+        "/favicon.ico",
+        "/index.html"


Does index.html have to be in here?

yes - Checked with Alex

gkalpak · 2017-12-06T16:46:24Z

aio/content/examples/service-worker-getstart/src/app/check-for-update.service.ts

+import { SwUpdate } from '@angular/service-worker';
+
+import { Observable } from 'rxjs/Observable';
+import 'rxjs/add/observable/interval';


It is better to replace the above 2 lines with: import { interval } from 'rxjs/observable/interval';

The app fails to compile when we remove the import Observable line. So keeping it as is.

It fails to compile, because you still mention Observable.interval below. It should be changed to just interval (in addition to replacing the above two lines per my previous comment.

gkalpak · 2017-12-06T16:50:27Z

Oh...too late 😞

kapunahelewong · 2017-12-06T17:11:42Z

@gkalpak @chembu is going to address these comments in a new PR. Handing this back to @chembu now.

gkalpak · 2017-12-06T19:12:48Z

Awesome! Feel free to ping me if you want me to take look.

kapunahelewong · 2017-12-06T19:29:47Z

@chembu ^^

angular-automatic-lock-bot · 2019-09-13T01:51:19Z

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 Dec 1, 2017

kapunahelewong force-pushed the wong-service-worker branch from 0aa146c to 6d1d3be Compare December 1, 2017 20:58

kapunahelewong force-pushed the wong-service-worker branch from 8efb6bc to e0d7fe3 Compare December 4, 2017 16:21

kapunahelewong force-pushed the wong-service-worker branch from 4522b3f to e0d7fe3 Compare December 4, 2017 17:20

IgorMinar added area: service-worker Issues related to the @angular/service-worker package comp: docs action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 5, 2017

kapunahelewong changed the title ~~[WIP] Troubleshooting: docs(aio): add service worker guide content and update nav~~ docs(aio): add service worker guide content and update nav Dec 5, 2017

kapunahelewong force-pushed the wong-service-worker branch from f945fe9 to 83fe3dc Compare December 5, 2017 20:36

kapunahelewong changed the title ~~docs(aio): add service worker guide content and update nav~~ docs(aio): add service worker guide Dec 5, 2017

alxhub approved these changes Dec 5, 2017

View reviewed changes

docs(aio): add service worker guide content and update nav

8dd050d

alxhub force-pushed the wong-service-worker branch from 83fe3dc to 8dd050d Compare December 5, 2017 23:32

alxhub requested a review from IgorMinar December 6, 2017 00:34

IgorMinar approved these changes Dec 6, 2017

View reviewed changes

IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 6, 2017

IgorMinar added the target: major This PR is targeted for the next major release label Dec 6, 2017

IgorMinar closed this in 9bbec42 Dec 6, 2017

gkalpak reviewed Dec 6, 2017

View reviewed changes

sreevani-ship-it mentioned this pull request Dec 11, 2017

docs(aio): rename service worker files, update examples, move SW under Techniques #20893

Merged

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

+                    });
+                });
+              });

docs(aio): add service worker guide #20736

docs(aio): add service worker guide #20736

Conversation

kapunahelewong commented Dec 1, 2017 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

mary-poppins commented Dec 1, 2017

mary-poppins commented Dec 1, 2017

mary-poppins commented Dec 1, 2017

mary-poppins commented Dec 4, 2017

mary-poppins commented Dec 4, 2017

mary-poppins commented Dec 4, 2017

mary-poppins commented Dec 4, 2017

mary-poppins commented Dec 4, 2017

mary-poppins commented Dec 4, 2017

mary-poppins commented Dec 5, 2017

mary-poppins commented Dec 5, 2017

mary-poppins commented Dec 5, 2017

mary-poppins commented Dec 5, 2017

IgorMinar left a comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Dec 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Dec 6, 2017

kapunahelewong commented Dec 6, 2017

gkalpak commented Dec 6, 2017

kapunahelewong commented Dec 6, 2017

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

kapunahelewong commented Dec 1, 2017 •

edited

gkalpak Dec 6, 2017 •

edited