-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Concerns about cache.addAll
not filtering non-200
#407
Comments
The solutions have a lot of magic. I would like a |
Yeah. Proposed solutions are weak, but the problem is very real. :) |
|
This seems like an enhancement. Not a priority for a minimum viable thing. |
Strongly disagree.
|
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. |
@wanderview how do you feel about: cache.addAll(urls, {
ensureOk: true
}); …that would fail if any response is opaque or There's probably a better name for it. |
@jakearchibald it seems if anything such a feature should be added to |
@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.) |
@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 |
@annevk I guess I looked at this as filtering of Anyway, I don't really care one way or the other. I'll let you two cage match over it. :-) |
Well |
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 |
Moving to whatwg/fetch#103 |
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 than200
Consequence: Dev uses
cache.addAll
expecting all resources to be fully operational on a successful outcome ofcache.addAll
. Yet sometimes offline apps which have reported a fully successful install fail for odd and hard to debug reasons. Dev ends up finding outcache.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 responsesConsequence: "That's OK, he thinks, I'll build a transactional function using
fetch
,put
andpromise.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 whycache.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:
cache.add
andcache.addAll
completely,allowNonCors
option is present.filter
option tocache.addAll
which is called with each request as first argument and allows to cancel the transaction (e.g. by throwing or returning false).The text was updated successfully, but these errors were encountered: