Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

The fetchAndCache function has a bug on mobile phones #275

Closed
303182519 opened this issue Nov 14, 2017 · 1 comment
Closed

The fetchAndCache function has a bug on mobile phones #275

303182519 opened this issue Nov 14, 2017 · 1 comment

Comments

@303182519
Copy link

303182519 commented Nov 14, 2017

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
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants