Create a new, non-navigate request when fetchOptions is set

[divyeshsachan]

Library Affected:
workbox-sw 3.6.1

Browser & Platform:
E.g. Chrome < v68 for Android & windows (not checked on other platforms but seems true for them as well).

Issue or Feature Request Description:
on event.request.mode == 'navigate', using fetchOptions for networkOnly, networkFirst strategy fails and throws offline fallback page with TypeError as follows. (not checked with other strategies like staleWhileRevalidate, etc.)

The problem is discussed in (https://stackoverflow.com/questions/46982502/cannot-construct-a-request-with-a-request-whose-mode-is-navigate-and-a-non-emp) and the selected answer points to remove fetchOptions completely when event.request.mode == 'navigate'. After removing RequestInit object i.e. fetchOptions from strategies works but defeats the purpose of fetchOptions completely.

So, it seems the limitation with fetch(), can you suggest a fix or possible workaround or overcome this issue on old chrome builds?

Most of my users using old chrome build on android as well as on windows platform are trapped into this issue and always see offline fallback page.

Your help is appreciated. Steps to reproduce issue follows

Network request for '/page2,html' threw an error. TypeError: Failed to execute 'fetch' on 'ServiceWorkerGlobalScope': Cannot construct a Request with a Request whose mode is 'navigate' and a non-empty RequestInit.
    at Object.<anonymous> (workbox-core.dev.js:1400)
    at Generator.next (<anonymous>)
    at step (workbox-core.dev.js:14)
    at workbox-core.dev.js:25

trace

_print | @ | workbox-core.dev.js:132
-- | -- | --
  | workbox.core.defaultExport.(anonymous function).args | @ | workbox-core.dev.js:149
  | (anonymous) | @ | workbox-core.dev.js:1407
  | step | @ | workbox-core.dev.js:14
  | (anonymous) | @ | workbox-core.dev.js:27
  | Promise.then (async) |   |  
  | step | @ | workbox-core.dev.js:24
  | (anonymous) | @ | workbox-core.dev.js:25
  | Promise.then (async) |   |  
  | step | @ | workbox-core.dev.js:24
  | self.babelHelpers.asyncToGenerator | @ | workbox-core.dev.js:32
  | self.babelHelpers.asyncToGenerator | @ | workbox-core.dev.js:11
  | wrappedFetch | @ | workbox-core.dev.js:1424
  | (anonymous) | @ | workbox-strategies.dev.js:822
  | step | @ | workbox-core.dev.js:14
  | self.babelHelpers.asyncToGenerator | @ | workbox-core.dev.js:32
  | self.babelHelpers.asyncToGenerator | @ | workbox-core.dev.js:11
  | makeRequest | @ | workbox-strategies.dev.js:850
  | (anonymous) | @ | workbox-strategies.dev.js:780
  | step | @ | workbox-core.dev.js:14
  | self.babelHelpers.asyncToGenerator | @ | workbox-core.dev.js:32
  | self.babelHelpers.asyncToGenerator | @ | workbox-core.dev.js:11
  | handle | @ | workbox-strategies.dev.js:784
  | defaultNetworkOnlyHandler | @ | sw.js:2
  | handleRequest | @ | workbox-routing.dev.js:365
  | workbox.routing.self.addEventListener.event | @ | workbox-routing.dev.js:835

This is modified sample code for your reference for easy reproduction of issue at your side. Activate the service worker with 'sw-offline' fallback page and navigate to other url. You will see a error log and returns offline fallback

	workbox.precaching.precache(['sw-offline']);
	workbox.precaching.addRoute({cleanUrls: false,directoryIndex: null});
	const defaultNetworkOnlyHandler = async (args) => {
		console.log('Default:'+args.url);
		try {
			var defaultNetworkOnly = workbox.strategies.networkOnly({
				fetchOptions: {credentials:'include'}
			});
			const response = await defaultNetworkOnly.handle(args);
			return response || await caches.match('sw-offline');
		} catch(error) {
			return await caches.match('sw-offline');
		}
	};
	workbox.routing.setDefaultHandler(defaultNetworkOnlyHandler);

[jeffposnick]

If you're making a navigation request, the event.request object should already have credentials set to 'include'. That's the default that browsers use for navigations:

When request’s mode is "navigate", its credentials mode is assumed to be "include" and fetch does not currently account for other values. If HTML changes here, this standard will need corresponding changes.

So I would expect that you'd get the behavior you want by not setting fetchOptions at all. That would result in this line

workbox/packages/workbox-core/_private/fetchWrapper.mjs

Line 113 in b146d8d

const fetchResponse = await fetch(request, fetchOptions);

being executed with fetchOptions set to undefined, and I believe that it will just use the request as-is (including with the existing credentials mode) in that case.

Or perhaps I'm missing something here related to older versions of Chrome that necessitates using fetchOptions? If so, could you clarify that bit?

[divyeshsachan]

The sample code i provided is only meant for reproduction of the issue. In real, i am appending additional header to each request to identify requests coming from service worker. Setting fetchOptions to undefined or {} when request.mode == 'navigate' works but defeats the purpose of setting additional header on each request.

The issue can be reproduced in older builds of Chrome, I've checked randomly in Chrome builds v60 to v65.

you may check with following code...

	workbox.precaching.precache(['sw-offline']);
	workbox.precaching.addRoute({cleanUrls: false,directoryIndex: null});
	const defaultNetworkOnlyHandler = async (args) => {
		console.log('Default:'+args.url);
		try {
			var myHeader = new Headers(event.request.headers);
						myHeader.append('SWVER', '1.0');
			var fetchOptions ={headers: myHeader};
			var defaultNetworkOnly = workbox.strategies.networkOnly({
				fetchOptions: fetchOptions
			});
			const response = await defaultNetworkOnly.handle(args);
			return response || await caches.match('sw-offline');
		} catch(error) {
			return await caches.match('sw-offline');
		}
	};
	workbox.routing.setDefaultHandler(defaultNetworkOnlyHandler);

One possibility to overcome this issue i can think of is using makeRequest, and create custom request object. And the following solution works.

	workbox.precaching.precache(['sw-offline']);
	workbox.precaching.addRoute({cleanUrls: false,directoryIndex: null});
	const defaultNetworkOnlyHandler = async (args) => {
		console.log('Default:'+args.url);
		try {
			var myHeader = new Headers(event.request.headers);
						myHeader.append('SWVER', '1.0');
			var fetchOptions ={headers: myHeader};
			var defaultNetworkOnly = workbox.strategies.networkOnly();
			var myRequest = await nRequest(args.event.request,fetchOptions);
			//console.log(myRequest);
			const response = await defaultNetworkOnly.makeRequest({args.event, request: myRequest});
			return response || await caches.match('sw-offline');
		} catch(error) {
			return await caches.match('sw-offline');
		}
	};
	workbox.routing.setDefaultHandler(defaultNetworkOnlyHandler);

	function nRequest(input, init) {
		var url;
		if (input instanceof Request) {
			url = input.url;

			init = init || {};
			Object.keys(Request.prototype).forEach(function (value) {
				if(input[value]&&!init[value]) init[value] = input[value];
			});
			delete init.url;
			init.mode = 'cors';
			return input.blob().then(function (blob) {
				if (input.method.toUpperCase() !== 'HEAD' && input.method.toUpperCase() !== 'GET' && blob.size > 0) {
					init.body = blob;
				}
				return new Request(url, init);
			})
		} else {
			url = url;
		}
		return new Request(url, init);
	}

So as per my opinion, in order to support fetchOptions properly for request.mode=='navigate' for all workbox strategies, workbox-core needs a immediate fix.

[jeffposnick]

One possibility to overcome this issue i can think of is using makeRequest, and create custom request object. And the following solution works.

Implementing your own handler that creates a non-navigate Request with custom headers seems like the right approach for this scenario. Your code seems reasonable.

So as per my opinion, in order to support fetchOptions properly for request.mode=='navigate' for all workbox strategies, workbox-core needs a immediate fix.

Are you asking for a change that would detect when fetchOptions is set and request.mode === 'navigate', and when that's true, automate the process of creating a new request with mode set to 'cors' instead? (Or set to 'same-origin', which seems more in keeping with what you'd expect from a navigation request?)

That could be done, but I'm not sure I'd describe that as a bug that necessitates an immediate fix as much as it's a feature request to save some effort of manually implementing the logic.

[divyeshsachan]

Are you asking for a change that would detect when fetchOptions is set and request.mode === 'navigate', and when that's true, automate the process of creating a new request with mode set to 'cors' instead? (Or set to 'same-origin', which seems more in keeping with what you'd expect from a navigation request?)

Yes, It'd be great to have this feature. It look me long time to think of possible solution to overcome this issue. I am sure many others have faced this issue while using workbox or native service-worker js code.

I'll not say it a bug, this is a fetch API limitation. In workbox, there should not be any limitation.

I think, while you make this feature request in next workbox release, current documentation of workbox strategies should be updated with this limitation. (because novice like me rely on documentation and when any limitation is not documented, it takes much time to actually dig root cause of the issue).

[Rainrider]

@jeffposnick
I think

workbox/packages/workbox-core/_private/fetchWrapper.mjs

Line 110 in d5c8277

pluginFilteredRequest = new Request(request, {mode: 'same-origin'});

will fail in Edge with

TypeError: Failed to construct 'Request': Invalid mode 'navigate'.

This seems to be not inline with the fetch spec (ref).

Please note this is from my tests yesterday, as I had the same issue and tried to work around it the same way you seem to address it in d5c8277, I haven't tested the beta release yet.

The solution for me was:

const req = event.request.clone();

const bodyPromise = 'body' in req ?
	Promise.resolve(req.body) :
	req.blob();
const body = await bodyPromise;

const request = new Request(req.url, {
	body: body,
	credentials: req.credentials,
	headers: req.headers,
	integrity: req.integrity,
	method: req.method,
	mode: 'same-origin',
	redirect: 'manual',
	referrer: req.referrer,
	referrerPolicy: req.referrerPolicy,
});

[jeffposnick]

Thanks for testing this out and letting me know. (Unfortunately, Edge is currently a big gap in our automated testing strategy.)

I can manually confirm that https://glitch.com/edit/#!/lace-staircase?path=sw.js:1:0 has problems in Edge.

We'll experiment with your approach and get something resolved for the final v4.0.0 release.

[jeffposnick]

FYI, rather than continue to experiment to find the most efficient approach, I'm going to put in a stop-gap fix described in #1862 targeting the v4.0.0 release, and we can iterate on that in a future release.

[philipwalton]

I'm going to remove the v4 milestone from this issue since I don't think we need to solve this before that major release.

[divyeshsachan]

@jeffposnick , @philipwalton, The function I've created is working well in all browsers, tested in Edge too.
It creates a new request using my custom nRequest function for all strategies, Have a look at it,

//init is actually fetchOptions
    function nRequest(input, init) {
        var url;
        if (input instanceof Request) {
            url = input.url;
            init = init || {};
            Object.keys(Request.prototype).forEach(function(value) {
                if (input[value] && !init[value]) init[value] = input[value]
            });
            delete init.url;
            if (init.mode == 'navigate') init.mode = 'same-origin';
            return input.blob().then(function(blob) {
                if (input.method.toUpperCase() !== 'HEAD' && input.method.toUpperCase() !== 'GET' && blob.size > 0) {
                    init.body = blob
                }
                return new Request(url, init)
            })
        } else {
            url = input
        }
        return new Request(url, init)
    }

//and it is passed to 
var myRequest = await nRequest(event.request, fetchOptions);
const response = await etStrategy.makeRequest({
                event,
                request: myRequest
            });

//where etStrategy is any workbox strategy using var etStrategy = new workbox.strategies.... 

For some personal reasons, I can't post the url of my sw.js file publicly. To see it in action, provide me a way to send you a private message. I'll be happy to show you my implementation and even full code.

[jeffposnick]

Thanks @divyeshsachan—I think we can point folks to that as a workaround if they really need this functionality for navigation requests in v4.0.0.

As it stands, it's a lot of code and I would rather not include it directly in workbox-core if we can avoid it, especially since it's mainly there to work around a browser compatibility bug that will likely go away in the future.

The most important thing in my mind is to ensure that we avoid failed navigations, and we do have that fix in for v4.0.0.

[tropicadri]

Since this was requested for WB v4, we are at v6 and have not seen movement in a while, I'll close the issue.