Concerns about cache.addAll not filtering non-200

[tobie]

AppCache, despite all of its flaws, guaranteed all resources transactionally cached had a status of 200.

For security reasons, cache.addAll can't give us these guarantees. Yet they-re critical to enable offline apps. Here are my concerns:

scenario 1: dev isn't aware cache.addAll accepts responses with a status other than 200

Consequence: Dev uses cache.addAll expecting all resources to be fully operational on a successful outcome of cache.addAll. Yet sometimes offline apps which have reported a fully successful install fail for odd and hard to debug reasons. Dev ends up finding out cache.addAll isn't really transactional to the extent he expected it to be, vows to never use it again, blames W3C for all the things and moves to native.

scenario 2: dev is aware cache.addAll accepts all kinds of responses

Consequence: "That's OK, he thinks, I'll build a transactional function using fetch, put and promise.all that works as expected." He then spends most of his days on Stackoverflow answering the questions of scenario 1 devs which don't understand why cache.addAll doesn't behave as expected; Writes a moderately successful library to fix SW issues; Gives a talk entitled "Service Workers are douchebags" and gets sued by the United Service Workers Union; blames W3C for all the things and moves to native.

Proposed solutions:

1. remove cache.add and cache.addAll completely,
2. Make filtering for a response status of 200 the default, throw on non-cors cross-origin request unless an allowNonCors option is present.
3. add a filter option to cache.addAll which is called with each request as first argument and allows to cancel the transaction (e.g. by throwing or returning false).

[annevk]

The solutions have a lot of magic. I would like a makeNon200ANetworkError flag (or maybe instead a list of acceptable status codes) that only works for same-origin or CORS. But adding a bunch of things that fetch() would not normally do seems somewhat odd and definitely worthy of extracting somehow.

[tobie]

Yeah. Proposed solutions are weak, but the problem is very real. :)

[jakearchibald]

only200 or onlyStatus: 200 could be an option on fetch & cache.add.

[annevk]

This seems like an enhancement. Not a priority for a minimum viable thing.

[tobie]

This seems like an enhancement. Not a priority for a minimum viable thing.

Strongly disagree.

cache.addAll is sugar, so it is itself an enhancement, not a requirement for v1/mvp. It is however a complete footgun the way it is currently specified. It's OK not to fix it for now, but in that case we shouldn't ship it at all.

[KenjiBaheux]

Add/AddAll is tracked by https://crbug.com/427652 for Blink

Blink has yet to implement add/addAll (the priority to do so is relatively low because these can be polyfilled rather easily).

Personally, I have a hard time to buy into what seems to be an extreme outcome given the fact that if you don't like the default behavior the API is powerful enough to let you implement what you wanted.

As to which is what most developers would expect, without data we could have the same argument over an add / addAll that special case non-200 responses.

I would argue that the brain-dead simple approach feels more natural: you ask us to add something, we'll do so (no questions asked) but if you want something smarter you can build it.

[jakearchibald]

@wanderview how do you feel about:

cache.addAll(urls, {
  ensureOk: true
});

…that would fail if any response is opaque or response.ok is false for any response?

There's probably a better name for it.

[annevk]

@jakearchibald it seems if anything such a feature should be added to Request, not here.

[wanderview]

@annevk this seems like something saying "the result of request needs to be X", which is more a function of the Response consumer than the Request itself.

@jakearchibald thats fine with me. I don't really have a strong opinion one way or another. Whatever is nicest for devs. (Sorry, that isn't very helpful.)

[annevk]

@wanderview how can it be a property of the response? Isn't what you want here the semantics of give a me a response that meets these conditions or a network error? That's the kind of thing Request does all the time.

[wanderview]

@annevk I guess I looked at this as filtering of cache.put(req, resp). This is about putting in, not getting out of cache.

Anyway, I don't really care one way or the other. I'll let you two cage match over it. :-)

[annevk]

Well addAll() first does a network fetch and you're placing conditions on the return value. If the condition is a non-match, you treat it identical to how you'd treat a network error. I don't really see how that does not make it a Request concern.

[jakearchibald]

Hmm, having it on request would avoid the…

fetch(url).then(response => {
  if (!response.ok) throw Error("NAH");
  // etc
})

…boilerplate. It would make the cache code:

cache.addAll(
  urls.map(u => new Request(u, {ensureOk: true}))
);

…which is a bit more verbose, but it would allow:

cache.addAll([
  '/hello/',
  '/world/',
  new Request('//other-origin/blah.txt', {mode: 'no-cors'})
].map(request => {
  if (request instanceof Request) return request;
  return new Request(url, {ensureOk: true});
}));

…meaning you could mix ensureOk requests with others.

[jakearchibald]

Moving to whatwg/fetch#103