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

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

Closed
borjapazr opened this issue Jun 11, 2019 · 3 comments

Comments

Projects
None yet
3 participants
@borjapazr
Copy link

commented Jun 11, 2019

Caching opaque responses with freshness strategy in ngsw-config.json [PWA][SW]

Relevant Package

This feature request is for @angular/pwa.

Description

The SW might have an option to cache opaque responses in the freshness mode since these caches are short-lived and therefore having an inadequate response would be quickly updated/removed.

I was reading ngsw-worker.js's code and now I have one question.
In the next function, what is the purpose of okToCacheOpaque? Is it to avoid having the opaque answers cached or just to the opposite? What is the real usage of this variable now?
Could this variable be used to cache opaque responses?

/**
         * Operation for caching the response from the server. This has to happen all
         * at once, so that the cache and LRU tracking remain in sync. If the network request
         * completes before the timeout, this logic will be run inline with the response flow.
         * If the request times out on the server, an error will be returned but the real network
         * request will still be running in the background, to be cached when it completes.
         */
        cacheResponse(req, res, lru, okToCacheOpaque = false) {
            return __awaiter$1(this, void 0, void 0, function* () {
                // Only cache successful responses.
                if (!res.ok || (okToCacheOpaque && res.type === 'opaque')) {
                    return;
                }
                // If caching this response would make the cache exceed its maximum size, evict something
                // first.
                if (lru.size >= this.config.maxSize) {
                    // The cache is too big, evict something.
                    const evictedUrl = lru.pop();
                    if (evictedUrl !== null) {
                        yield this.clearCacheForUrl(evictedUrl);
                    }
                }
                // TODO: evaluate for possible race conditions during flaky network periods.
                // Mark this resource as having been accessed recently. This ensures it won't be evicted
                // until enough other resources are requested that it falls off the end of the LRU chain.
                lru.accessed(req.url);
                // Store the response in the cache (cloning because the browser will consume
                // the body during the caching operation).
                yield (yield this.cache).put(req, res.clone());
                // Store the age of the cache.
                const ageTable = yield this.ageTable;
                yield ageTable.write(req.url, { age: this.adapter.time });
                // Sync the LRU chain to non-volatile storage.
                yield this.syncLru();
            });
        }

Describe the solution you'd like

Caching opaque responses in the freshness mode of dataGroups in ngsw-config.json

Describe alternatives you've considered

https://medium.com/@sascha.wolff/how-to-use-angular-6-service-worker-for-dynamically-loading-images-f4f6f497699f

@borjapazr borjapazr changed the title Support caching of opaque responses with freshness strategy in ngsw-config.json [PWA][SW] Support caching opaque responses with freshness strategy in ngsw-config.json [PWA][SW] Jun 11, 2019

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

gkalpak added a commit to gkalpak/angular that referenced this issue Jun 11, 2019

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 angular#30968
@gkalpak

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

You are right, @borjapazr. It looks like a bug in the if condition (since okToCacheOpaque is never taken into account atm, because opaque responses have res.ok === false). I assume the intention was to negate the condition:

-if (!res.ok || (okToCacheOpaque && res.type === 'opaque')) {
+if (!(res.ok || (okToCacheOpaque && res.type === 'opaque'))) {

I have a fix for that in #30977 (see 7352a7f), but we should also add some docs about this (e.g. similar to workbox's).

@alxhub

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@gkalpak that change looks right to me.

@borjapazr

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

You are right, @borjapazr. It looks like a bug in the if condition (since okToCacheOpaque is never taken into account atm, because opaque responses have res.ok === false). I assume the intention was to negate the condition:

-if (!res.ok || (okToCacheOpaque && res.type === 'opaque')) {
+if (!(res.ok || (okToCacheOpaque && res.type === 'opaque'))) {

I have a fix for that in #30977 (see 7352a7f), but we should also add some docs about this (e.g. similar to workbox's).

Now it makes sense to me. Thank you @gkalpak.

And adding more documentation would be a great idea 😊

gkalpak added a commit to gkalpak/angular that referenced this issue Jun 24, 2019

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 angular#30968

gkalpak added a commit to gkalpak/angular that referenced this issue Jun 24, 2019

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 angular#30968

alxhub added a commit that referenced this issue 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 alxhub closed this in d7be38f 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.