GoogleChromeLabs / sw-toolbox

[Deprecated] A collection of service worker tools for offlining runtime requests
https://developers.google.com/web/tools/workbox/guides/migrations/migrate-from-sw
Apache License 2.0
3.61k stars 331 forks source link

The fetchAndCache function has a bug on mobile phones #275

Closed 303182519 closed 7 years ago

303182519 commented 7 years ago

The original code can not pass through the machine's cache

function fetchAndCache(request, options) {
  options = options || {};
  var successResponses = options.successResponses ||
      globalOptions.successResponses;

  return fetch(request.clone()).then(function(response) {
    // Only cache GET requests with successful responses.
    // Since this is not part of the promise chain, it will be done
    // asynchronously and will not block the response from being returned to the
    // page.
    if (request.method === 'GET' && successResponses.test(response.status)) {
      openCache(options).then(function(cache) {
        cache.put(request, response).then(function() {
          // If any of the options are provided in options.cache then use them.
          // Do not fallback to the global options for any that are missing
          // unless they are all missing.
          var cacheOptions = options.cache || globalOptions.cache;

          // Only run the cache expiration logic if at least one of the maximums
          // is set, and if we have a name for the cache that the options are
          // being applied to.
          if ((cacheOptions.maxEntries || cacheOptions.maxAgeSeconds) &&
              cacheOptions.name) {
            queueCacheExpiration(request, cache, cacheOptions);
          }
        });
      });
    }

    return response.clone();
  });
}

This is after the amendment

function fetchAndCache(request, options) {
    options = options || {};
    var successResponses = options.successResponses ||
            globalOptions.successResponses;

    var url = new URL(request.clone().url);

    url.search += (url.search ? '&swTime=' : '?swTime=') + Date.now();

    return fetch(url.toString()).then(function(response) {
        // Only cache GET requests with successful responses.
        // Since this is not part of the promise chain, it will be done
        // asynchronously and will not block the response from being returned to the
        // page.
        if (request.method === 'GET' && successResponses.test(response.status)) {
            openCache(options).then(function(cache) {
                cache.put(request, response).then(function() {
                    // If any of the options are provided in options.cache then use them.
                    // Do not fallback to the global options for any that are missing
                    // unless they are all missing.
                    var cacheOptions = options.cache || globalOptions.cache;

                    // Only run the cache expiration logic if at least one of the maximums
                    // is set, and if we have a name for the cache that the options are
                    // being applied to.
                    if ((cacheOptions.maxEntries || cacheOptions.maxAgeSeconds) &&
                            cacheOptions.name) {
                        queueCacheExpiration(request, cache, cacheOptions);
                    }
                });
            });
        }

        return response.clone();
    });
}
jeffposnick commented 7 years ago

I'm not sure what this has to do with mobile phones, as the main change that I could see has to do with the addition of cache-busting URL parameters to the fetch() request.

The original code's lack of cache-busting when performing a fetch() in sw-toolbox is intentional.

The expectation is that the appropriate Cache-Control headers should be used when responding to requests, whether they're requests from a service worker or requests from an uncontrolled web page.