Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
4 participants
@gkalpak
Copy link
Member

commented Jun 11, 2019

  • 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

This comment has been minimized.

Copy link

commented Jun 11, 2019

@gkalpak gkalpak force-pushed the gkalpak:fix-sw-caching-opaque branch from 84d287d to d7dab23 Jun 24, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 24, 2019

@IgorMinar
Copy link
Member

left a comment

Left a few comments but otherwise lgtm. Thanks!

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

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 24, 2019

Member

access is an odd name for this const

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 24, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 24, 2019

Member

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 24, 2019

Author Member

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.

gkalpak added some commits Jun 24, 2019

test(service-worker): remove obsolete async test helpers
Jasmine natively supports returning promises from spec functions for
quite some time now. We don't need special async helpers.
test(service-worker): better simulate opaque requests
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
refactor(service-worker): make the caching behavior more explicit
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
fix(service-worker): cache opaque responses in data groups with `fres…
…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 #30968
refactor(service-worker): remove redundant cache operation
At this point, the response will have been cached (or scheduled to be
cached) in other code paths, so caching it again is redundant.

@gkalpak gkalpak force-pushed the gkalpak:fix-sw-caching-opaque branch from d7dab23 to b8b61c7 Jun 24, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 24, 2019

@gkalpak

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

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

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

test(service-worker): remove obsolete async test helpers (#30977)
Jasmine natively supports returning promises from spec functions for
quite some time now. We don't need special async helpers.

PR Close #30977

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

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

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

test(service-worker): better simulate opaque requests (#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

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

refactor(service-worker): make the caching behavior more explicit (#3…
…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

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

fix(service-worker): cache opaque responses in data groups with `fres…
…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

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

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

refactor(service-worker): remove redundant cache operation (#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

@alxhub alxhub closed this in b6e8d19 Jun 27, 2019

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

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

test(service-worker): better simulate opaque requests (#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

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

refactor(service-worker): make the caching behavior more explicit (#3…
…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

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

fix(service-worker): cache opaque responses in data groups with `fres…
…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

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

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

refactor(service-worker): remove redundant cache operation (#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

@gkalpak gkalpak deleted the gkalpak:fix-sw-caching-opaque branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.