fix(service-worker): cache opaque responses in data groups with `freshness` strategy #30977

gkalpak · 2019-06-11T16:47:59Z

Fix caching of opaque responses in data groups with freshness strategy.
Clean-up/Simplify service-worker tests.

(See individual commits for details.)

Fixes #30968.

mary-poppins · 2019-06-11T16:54:57Z

You can preview 84d287d at https://pr30977-84d287d.ngbuilds.io/.

mary-poppins · 2019-06-24T12:45:05Z

You can preview d7dab23 at https://pr30977-d7dab23.ngbuilds.io/.

IgorMinar

Left a few comments but otherwise lgtm. Thanks!

IgorMinar · 2019-06-24T14:19:51Z

packages/service-worker/worker/testing/fetch.ts

-      throw new Error('No body');
+    const body = this.getBody();
+    const buffer = new ArrayBuffer(body.length);
+    const access = new Uint8Array(buffer);


access is an odd name for this const

I didn't change this bit (just removed the surrounding if block and adjusted the indentation).
But happy to change it so something else (view sounds like a good name).

IgorMinar · 2019-06-24T14:28:45Z

packages/service-worker/worker/src/data.ts

    // Only cache successful responses.
-    if (!res.ok || (okToCacheOpaque && res.type === 'opaque')) {
+    if (!(res.ok || (okToCacheOpaque && res.type === 'opaque'))) {


Isn't this going to cause non-200 opaque responses to be cached?

Yes (if okToCacheOpaque is true, which is only the case when handling a DataGroup request with freshness (i.e. network first)).

With opaque responses, one can't tell if it was successful or not (res.status is 0, thus res.ok will always be false). So, what we do is that we cache opaque responses for DataGroup requests that are configured to fetch from the network first. So, if the client is online, the SW should fetch the response from the network anyway (and not use the cache), but if the client is offline, the SW will serve the cached response. There is little risk (other than the extra memory), because the request would have failed anyway when offline.

Jasmine natively supports returning promises from spec functions for quite some time now. We don't need special async helpers.

Previously, opaque responses where handled a little differently than other responses from the mock server. More specifically, they were not tracked (so no assertions could be made for them) and their [`Body` mixin][1] methods (such as `arrayBuffer()`, `json()`, `text()`) would throw an error due to `body` being `null`. This commit ensures opaque responses are also captured on the mock server and also changes `Body` mixin methods to better simulate the [spec'd behavior][2]. (These improvements will be necessary to test caching of opaque responses in a subsequent commit.) [1]: https://developer.mozilla.org/en-US/docs/Web/API/Body [2]: https://fetch.spec.whatwg.org/#concept-body-consume-body

This commit doesn't change the behavior wrt caching, but it makes it more explicit that only non-timed-out responses are cached. In case of a timeout, `res` would be set to a programmatically created 504 `Response`, so `cacheResponse()` (which checks for `res.ok`) would not have cached it anyway, but this makes change makes it more explicit (and more similar to the equivalent part in [handleFetchWithFreshness()][1]). [1]: https://github.com/angular/angular/blob/2b4d5c754/packages/service-worker/worker/src/data.ts#L379-L388

…hness` strategy Previously, (presummably due to a typo) the `okToCacheOpaque` argument of `DataGroup#cacheResponse()` was essentially never taken into account (since opaque responses have a non-200 status code and thus `res.ok` is always false). This commit fixes the typo, which allows opaque responses to be cached when `okToCacheOpaque` is true (i.e. in data groups using the `freshness` strategy). Fixes angular#30968

…eout threshold

At this point, the response will have been cached (or scheduled to be cached) in other code paths, so caching it again is redundant.

mary-poppins · 2019-06-24T20:20:39Z

You can preview b8b61c7 at https://pr30977-b8b61c7.ngbuilds.io/.

gkalpak · 2019-06-24T20:26:19Z

Even if this is theoretically a fix, the code never worked as intended (i.e. opaque responses were never cached), so this is a change in behavior. Therefore, I marked it for master-only. (@IgorMinar, feel free to mark for master & patch if you think that is more appropriate.)

gkalpak · 2019-06-26T21:36:33Z

Changing to master & patch, 8.1.x is the new patch branch.

Jasmine natively supports returning promises from spec functions for quite some time now. We don't need special async helpers. PR Close #30977

PR Close #30977

…30977) PR Close #30977

Previously, opaque responses where handled a little differently than other responses from the mock server. More specifically, they were not tracked (so no assertions could be made for them) and their [`Body` mixin][1] methods (such as `arrayBuffer()`, `json()`, `text()`) would throw an error due to `body` being `null`. This commit ensures opaque responses are also captured on the mock server and also changes `Body` mixin methods to better simulate the [spec'd behavior][2]. (These improvements will be necessary to test caching of opaque responses in a subsequent commit.) [1]: https://developer.mozilla.org/en-US/docs/Web/API/Body [2]: https://fetch.spec.whatwg.org/#concept-body-consume-body PR Close #30977

…0977) This commit doesn't change the behavior wrt caching, but it makes it more explicit that only non-timed-out responses are cached. In case of a timeout, `res` would be set to a programmatically created 504 `Response`, so `cacheResponse()` (which checks for `res.ok`) would not have cached it anyway, but this makes change makes it more explicit (and more similar to the equivalent part in [handleFetchWithFreshness()][1]). [1]: https://github.com/angular/angular/blob/2b4d5c754/packages/service-worker/worker/src/data.ts#L379-L388 PR Close #30977

…hness` strategy (#30977) Previously, (presummably due to a typo) the `okToCacheOpaque` argument of `DataGroup#cacheResponse()` was essentially never taken into account (since opaque responses have a non-200 status code and thus `res.ok` is always false). This commit fixes the typo, which allows opaque responses to be cached when `okToCacheOpaque` is true (i.e. in data groups using the `freshness` strategy). Fixes #30968 PR Close #30977

…eout threshold (#30977) PR Close #30977

At this point, the response will have been cached (or scheduled to be cached) in other code paths, so caching it again is redundant. PR Close #30977

PR Close #30977

…30977) PR Close #30977

Previously, opaque responses where handled a little differently than other responses from the mock server. More specifically, they were not tracked (so no assertions could be made for them) and their [`Body` mixin][1] methods (such as `arrayBuffer()`, `json()`, `text()`) would throw an error due to `body` being `null`. This commit ensures opaque responses are also captured on the mock server and also changes `Body` mixin methods to better simulate the [spec'd behavior][2]. (These improvements will be necessary to test caching of opaque responses in a subsequent commit.) [1]: https://developer.mozilla.org/en-US/docs/Web/API/Body [2]: https://fetch.spec.whatwg.org/#concept-body-consume-body PR Close #30977

…0977) This commit doesn't change the behavior wrt caching, but it makes it more explicit that only non-timed-out responses are cached. In case of a timeout, `res` would be set to a programmatically created 504 `Response`, so `cacheResponse()` (which checks for `res.ok`) would not have cached it anyway, but this makes change makes it more explicit (and more similar to the equivalent part in [handleFetchWithFreshness()][1]). [1]: https://github.com/angular/angular/blob/2b4d5c754/packages/service-worker/worker/src/data.ts#L379-L388 PR Close #30977

…hness` strategy (#30977) Previously, (presummably due to a typo) the `okToCacheOpaque` argument of `DataGroup#cacheResponse()` was essentially never taken into account (since opaque responses have a non-200 status code and thus `res.ok` is always false). This commit fixes the typo, which allows opaque responses to be cached when `okToCacheOpaque` is true (i.e. in data groups using the `freshness` strategy). Fixes #30968 PR Close #30977

…eout threshold (#30977) PR Close #30977

At this point, the response will have been cached (or scheduled to be cached) in other code paths, so caching it again is redundant. PR Close #30977

angular-automatic-lock-bot · 2019-09-15T06:22:04Z

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

gkalpak added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer PR target: TBD area: service-worker Issues related to the @angular/service-worker package labels Jun 11, 2019

gkalpak requested a review from a team as a code owner June 11, 2019 16:47

ngbot bot added this to the needsTriage milestone Jun 11, 2019

googlebot added the cla: yes label Jun 11, 2019

gkalpak mentioned this pull request Jun 11, 2019

Support caching opaque responses with freshness strategy in ngsw-config.json [PWA][SW] #30968

Closed

gkalpak force-pushed the fix-sw-caching-opaque branch from 84d287d to d7dab23 Compare June 24, 2019 12:04

IgorMinar approved these changes Jun 24, 2019

View reviewed changes

gkalpak added 9 commits June 24, 2019 23:13

test(service-worker): remove obsolete async test helpers

cb71a13

Jasmine natively supports returning promises from spec functions for quite some time now. We don't need special async helpers.

test(service-worker): simplify test helpers

a725e1d

test(service-worker): properly reset mock server state for each test

60c406b

fix(service-worker): cache opaque responses when requests exceeds tim…

be5109a

…eout threshold

refactor(service-worker): remove redundant cache operation

9f10975

At this point, the response will have been cached (or scheduled to be cached) in other code paths, so caching it again is redundant.

fixup! test(service-worker): better simulate opaque requests

b8b61c7

gkalpak force-pushed the fix-sw-caching-opaque branch from d7dab23 to b8b61c7 Compare June 24, 2019 20:14

gkalpak added target: major This PR is targeted for the next major release and removed PR target: TBD labels Jun 24, 2019

alxhub pushed a commit that referenced this pull request Jun 27, 2019

test(service-worker): remove obsolete async test helpers (#30977)

d0e213d

Jasmine natively supports returning promises from spec functions for quite some time now. We don't need special async helpers. PR Close #30977

alxhub pushed a commit that referenced this pull request Jun 27, 2019

test(service-worker): simplify test helpers (#30977)

642f604

PR Close #30977

alxhub pushed a commit that referenced this pull request Jun 27, 2019

test(service-worker): properly reset mock server state for each test (#…

54c171c

…30977) PR Close #30977

alxhub pushed a commit that referenced this pull request Jun 27, 2019

fix(service-worker): cache opaque responses when requests exceeds tim…

a9038ef

…eout threshold (#30977) PR Close #30977

alxhub closed this in b6e8d19 Jun 27, 2019

alxhub pushed a commit that referenced this pull request Jun 27, 2019

test(service-worker): simplify test helpers (#30977)

7217525

PR Close #30977

alxhub pushed a commit that referenced this pull request Jun 27, 2019

test(service-worker): properly reset mock server state for each test (#…

c150354

…30977) PR Close #30977

alxhub pushed a commit that referenced this pull request Jun 27, 2019

fix(service-worker): cache opaque responses when requests exceeds tim…

93abc35

…eout threshold (#30977) PR Close #30977

gkalpak deleted the fix-sw-caching-opaque branch June 27, 2019 18:30

iget-master mentioned this pull request Jul 10, 2019

CHANGELOG.md shouldn't include pre-release changes? #31491

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(service-worker): cache opaque responses in data groups with `freshness` strategy #30977

fix(service-worker): cache opaque responses in data groups with `freshness` strategy #30977

gkalpak commented Jun 11, 2019

mary-poppins commented Jun 11, 2019

mary-poppins commented Jun 24, 2019

IgorMinar left a comment

IgorMinar Jun 24, 2019

gkalpak Jun 24, 2019

IgorMinar Jun 24, 2019

gkalpak Jun 24, 2019

mary-poppins commented Jun 24, 2019

gkalpak commented Jun 24, 2019

gkalpak commented Jun 26, 2019

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

fix(service-worker): cache opaque responses in data groups with freshness strategy #30977

fix(service-worker): cache opaque responses in data groups with freshness strategy #30977

Conversation

gkalpak commented Jun 11, 2019

mary-poppins commented Jun 11, 2019

mary-poppins commented Jun 24, 2019

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar Jun 24, 2019

Choose a reason for hiding this comment

gkalpak Jun 24, 2019

Choose a reason for hiding this comment

IgorMinar Jun 24, 2019

Choose a reason for hiding this comment

gkalpak Jun 24, 2019

Choose a reason for hiding this comment

mary-poppins commented Jun 24, 2019

gkalpak commented Jun 24, 2019

gkalpak commented Jun 26, 2019

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

fix(service-worker): cache opaque responses in data groups with `freshness` strategy #30977

fix(service-worker): cache opaque responses in data groups with `freshness` strategy #30977