fix(service-worker): some fixes for real-world apps by alxhub · Pull Request #19519 · angular/angular

alxhub · 2017-10-02T23:11:44Z

No description provided.

mary-poppins · 2017-10-02T23:24:25Z

You can preview a09ae1f at https://pr19519-a09ae1f.ngbuilds.io/.

mary-poppins · 2017-10-02T23:34:54Z

You can preview 83ba832 at https://pr19519-83ba832.ngbuilds.io/.

gkalpak

The public API needs some changes.
(Also, how come there are no tests for the changes? 😕)

gkalpak · 2017-10-03T07:50:49Z

OOC, what is the purpose of this?

gkalpak · 2017-10-03T07:59:39Z

If this is supposed to only exclude /, then escaping is not necessary (since you are inside a character group): [^/]+.

Good point. I don't mind being explicit about it though :)

gkalpak · 2017-10-03T08:06:19Z

Not directly related to this PR, but is it intentional that the generated RegExp will happily match a path as long as it can match a sub-path in it. For example, the RegExp for glob /foo/**/bar.js will match /baz/foo/qux/bar.js and /foo/qux/bar.jsx.

Yes, this is intentional. ** means "0 or more directories", * means "1 directory".

I was referring to the fact the two example paths (/baz/foo/qux/bar.js and /foo/qux/bar.jsx) would normally not be matched by the glob; the former because of the /baz prefix and the latter because of the x suffix (in .jsx). In both paths /foo/qux/bar.js would be matched, but it will only be a substring of the whole path.

Oh. Yes, that's expected. The expression is wrapped in^ and $ before being compiled. This function doesn't include those matchers because patterns need to be encoded into the manifest in an unwrapped state.

gkalpak · 2017-10-03T08:14:08Z

Are there other glob features supported that might need escaping (e.g. {...}, [...], ^)?

Those are intentionally not handled for now. Long-term, I'd like to switch to use a glob-to-regex library that already exists.

gkalpak · 2017-10-03T08:39:03Z

gkalpak · 2017-10-03T08:49:31Z

Why do we need two of theses sha1 files? Is it for bundling purposes?

Yeah, it's for build purposes. There's also a copy in Angular itself. :(

I want to extract it into a separate library at some point.

gkalpak · 2017-10-03T08:51:35Z

Doesn't this need the this.bodyUsed = true; line?

gkalpak · 2017-10-03T08:54:03Z

The res.url !== '' part is redundant 😁

gkalpak · 2017-10-03T08:59:32Z

I am a little confused with req.url vs getConfigUrl(req.url) 😕

For example, should line 450 below use url instead? Or what kind of URLs are used when initializing an AppVersion's hashTable (and if not "parsed" URLs, why not)?

https://github.com/alxhub/angular/blob/83ba8320e8937c5655d609cd2938295d724bec03/packages/service-worker/worker/src/app-version.ts#L54-L56

The issue is that the config generator has no knowledge of which origin it will be running on. The SW knows its origin, but not whether requests will be made via HTTP or HTTPS.

Therefore there are two types of URLs tracked in the hashTable and other internal structures - fully qualified (with protocol, domain, and path for everything known at build time) and relative to the domain root (everything loaded from the same origin as the SW, and so not known at build time).

The URLs that come in Requests are always fully qualified. getConfigUrl will convert that fully-qualified URL to a domain-root-relative URL if the origin is the same as the SW. In other words, it determines for a given URL how it would be listed in the manifest.

In this case, it probably should use the config URL.

mary-poppins · 2017-10-03T20:46:51Z

You can preview f83e2e6 at https://pr19519-f83e2e6.ngbuilds.io/.

mary-poppins · 2017-10-04T16:52:52Z

You can preview 99c7768 at https://pr19519-99c7768.ngbuilds.io/.

mary-poppins · 2017-10-04T16:56:21Z

You can preview d639ad0 at https://pr19519-d639ad0.ngbuilds.io/.

mary-poppins · 2017-10-04T18:19:03Z

You can preview 5b3ca03 at https://pr19519-5b3ca03.ngbuilds.io/.

gkalpak · 2017-10-04T18:27:40Z

a Response, an error orundefined --> a Response or undefined (right?)

Correct. Fixed.

gkalpak · 2017-10-04T18:35:45Z

There is intentionally no await here, right?

Correct. return foo; and return await foo; are identical.

I don't think they are identical if this.scope.fetch(...) rejects 😕

gkalpak · 2017-10-04T19:34:59Z

Did you mean debugLogB?

I do indeed.

gkalpak · 2017-10-04T20:06:16Z

Is origin guaranteed to end with a /? If not, then url would end up with a a double //.

Since this is testing code, yes it is :)

gkalpak · 2017-10-04T20:52:34Z

Is this intentionally inside the while loop?

Good point. It's only used for tests anyway - fixed.

mary-poppins · 2017-10-04T23:48:16Z

You can preview 86454df at https://pr19519-86454df.ngbuilds.io/.

mary-poppins · 2017-10-05T15:44:31Z

You can preview 5839d90 at https://pr19519-5839d90.ngbuilds.io/.

mary-poppins · 2017-10-05T15:45:09Z

You can preview 9be4f1d at https://pr19519-9be4f1d.ngbuilds.io/.

IgorMinar · 2017-10-05T16:38:40Z

can't you console.error here?

I try to avoid console logging for the SW - it's occasionally difficult to get to the SW debugging window. I prefer to expose debugging info via a special /ngsw/* URL namespace.

IgorMinar

lgmt for api changes. please add tests as discussed in person.

This commit fixes several issues discovered through use in real apps. * The sha1() function operated on text content, causing issues for binary-format files. A sha1Binary() function which operates on unparsed data now avoids any encoding issues. * The characters '?' and '+' were not escaped in Glob-to-regex conversion previously, but are now. * URLs from the browser contain the full origin, but were checked against the table of hashes from the manifest which only has the path for URLs from the same origin. Now the origin is checked and URLs are relativized to the domain root before comparison if appropriate. * ngsw: prefix was missing from data groups, is now added. * Occasionally servers will return a redirected response for an asset, and caching it could cause errors for navigation requests. The SW now handles this by detecting such responses and following the redirect manually, to avoid caching a redirected response. * The request for known assets is now created from scratch from the URL before fetching from the network, in order to sanitize it and avoid carrying any special modes or headers that might result in opaque responses. * Debugging log for troubleshooting. * Avoid creating errors by returning 504 responses on error. * Fix bug where idle queue doesn't run in some circumstances. * Add tests for the above.

mary-poppins · 2017-10-05T18:08:20Z

You can preview bdeb92b at https://pr19519-bdeb92b.ngbuilds.io/.

gkalpak · 2017-10-05T20:43:24Z

+
+  private async safeFetch(req: Request): Promise<Response> {
+    try {
+      return this.scope.fetch(req);


AFAICT, this should also be return await this.scope.fetch(req).

angular-automatic-lock-bot · 2019-09-12T17:52:44Z

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 2, 2017

alxhub force-pushed the new-sw-fixes branch from a09ae1f to 83ba832 Compare October 2, 2017 23:25

gkalpak added the area: service-worker Issues related to the @angular/service-worker package label Oct 3, 2017

gkalpak reviewed Oct 3, 2017

View reviewed changes

alxhub force-pushed the new-sw-fixes branch from 83ba832 to f83e2e6 Compare October 3, 2017 20:36

alxhub force-pushed the new-sw-fixes branch from 99c7768 to d639ad0 Compare October 4, 2017 16:47

alxhub force-pushed the new-sw-fixes branch from d639ad0 to 5b3ca03 Compare October 4, 2017 18:09

alxhub requested a review from IgorMinar October 4, 2017 18:10

gkalpak reviewed Oct 4, 2017

View reviewed changes

alxhub force-pushed the new-sw-fixes branch from 5b3ca03 to 86454df Compare October 4, 2017 23:38

alxhub force-pushed the new-sw-fixes branch 3 times, most recently from 5839d90 to 9be4f1d Compare October 5, 2017 15:35

IgorMinar reviewed Oct 5, 2017

View reviewed changes

IgorMinar approved these changes Oct 5, 2017

View reviewed changes

alxhub force-pushed the new-sw-fixes branch from 9be4f1d to bdeb92b Compare October 5, 2017 17:46

alxhub merged commit f10f8db into angular:master Oct 5, 2017

gkalpak reviewed Oct 5, 2017

View reviewed changes

webmaxru mentioned this pull request Oct 26, 2017

Service Worker: Offline mode for assetGroups works unstable #19490

Closed

webmaxru mentioned this pull request Oct 26, 2017

Service Worker: Freshness strategy doesn't work #19489

Closed

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

Conversation

alxhub commented Oct 2, 2017

Uh oh!

mary-poppins commented Oct 2, 2017

Uh oh!

mary-poppins commented Oct 2, 2017

Uh oh!

gkalpak 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!

gkalpak Oct 4, 2017 • 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!

gkalpak Oct 3, 2017 • 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!

mary-poppins commented Oct 3, 2017

Uh oh!

mary-poppins commented Oct 4, 2017

Uh oh!

mary-poppins commented Oct 4, 2017

Uh oh!

mary-poppins commented Oct 4, 2017

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!

gkalpak Oct 4, 2017 •

edited

Loading

gkalpak Oct 3, 2017 •

edited

Loading