toolbox.precache should not cache invalid files

[gauntface]

trafficstars

At the moment toolbox.precache will cache a 404 response.

My initial assumption this in unexpected default behavior (and not something I think would be desirable in any situation).

Would allowing 20x status be a better option?

The concern this raises is what is the behavior when a 404 is encountered? I would expect the service worker install to fail.

[jeffposnick]

The background on the current behavior is covered in this issue from the old repo: https://github.com/wibblymat/shed/issues/5

The current configuration is at https://github.com/GoogleChrome/sw-toolbox/blob/master/lib/options.js#L39 and developers could technically override the defaults using whatever RegExp they'd prefer, though I'm sure there's no one who actually does that.

My vote is to keep the current behavior, but I can definitely see the justification for the other side.

[gauntface]

That bug is in reference to networkFirst behaviour, not the precache behaviour.

The obvious major downside of this is the progressive web app story where you may need and rely on a file that doesn't get cached. In that scenario I'd expect the sw to fail to install.

Appreciate it can be overriden but I don't think this default behaviour is good or expected.

cc @jakearchibald because I know there was discussion around behaviour or cache.addAll and caching 404s.

[wibblymat]

Overall I think I'm in favour of being stricter in the precache. A successful install should mean that the app is ready for offline. However, playing Devil's Advocate, here are some cases where the current behaviour is possibly better:

• Caching a custom 404 page. Maybe if you have routes for all of the real URLs on your site you could have the default route just serve up a 404 without checking the server. Precache would fail when it successfully fetched the 404 page if it's served with the correct status code.
• To avoid masking a problem. Let's say that your list of precache resources has a typo in it for some resource. With the proposed behaviour none of your existing users would be able to upgrade, but they also wouldn't hit any errors. It might be some time to even notice that users don't have the latest version. By precaching the 404 you will get bug reports from users much quicker.

[gauntface]

404

Caching 404 pages is an interesting use case, I'd see two options:

1. Developer creates an endpoint of a query parameter that returns 404 with a 200 status code
2. precache has an additional method that allows the developer to override a valid or invalid response.

This could look something like:

swtoolbox.precache([
    ' /valid',
    '/my-404'
], (asset, response, isValid) => {
    if (asset == '/my-404') {
        return true;
     }
     return isValid; 
});

Nice thing with a callback function is that the developer is given a lot of control if they want it.

Failing new SW

Few things that weren't obvious with this:

• How will push subscriptions be treated with this?
• For site developers I imagine two general use cases will exist:
  • I want my site to work offline and be reliable
  • I want SW to help with perf and maybe offline, but I want it to be up to date

My main thoughts were to provide an option that basically says if the install fails fallback to network. I'm thinking of an indexdb value that indicates that a new toolbox installed failed. This would allow all of the above options.

A different consideration is that if developers are concerned with this behaviour, they could just return true in precache callback and then they'll always get the latest SW. Not ideal, but for some it may be an ok option.

[gauntface]

General discussion:

• Possible to add a catch to the end of precache or some error handling to send a response to analytics
• 404, Possibly add a callback to override if a response if valid or not.

[NekR]

If sw-toolbox is using addAll() (and this seems it does: https://github.com/GoogleChrome/sw-toolbox/blob/4d172db13087d5727c6a3fe73a9341e6f2c4b5e4/lib/sw-toolbox.js#L65) then probably problem is in that method, if I got it right.

Here is PR to fix that: https://github.com/coonsta/cache-polyfill/pull/19 And the discussion about addAll()/add() behavior: https://github.com/slightlyoff/ServiceWorker/issues/823

[gauntface]

@NekR thanks for chiming in.

The problem isn't around addAll (although I'm aware that it's behaviour will affect this).

It's a discussion of the regex used for caching as @jeffposnick mentioned: https://github.com/GoogleChrome/sw-toolbox/blob/master/lib/options.js#L39

[NekR]

@gauntface sorry, I thought it's about addAll() (saw it somewhere in comments).

[samuelli]

For us, adding 404s was weird unexpected behavior even as a developer.