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 · 4 comments
Labels
area: service-worker Issues related to the @angular/service-worker package type: bug/fix
Milestone

Comments

@borjapazr
Copy link
Contributor

borjapazr 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
@gkalpak gkalpak added area: service-worker Issues related to the @angular/service-worker package type: bug/fix labels 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
…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
Copy link
Member

gkalpak 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
Copy link
Member

alxhub commented Jun 11, 2019

@gkalpak that change looks right to me.

@borjapazr
Copy link
Contributor Author

borjapazr 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
…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
…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 pushed a commit that referenced this issue Jun 27, 2019
…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 as completed in d7be38f Jun 27, 2019
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: service-worker Issues related to the @angular/service-worker package type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants