-
Notifications
You must be signed in to change notification settings - Fork 315
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Change add(request) and addAll(requests) behavior
- Change add and addAll to reject if any of the responses are not in ok status - Automatically disallow opaque responses and opaque-redirect responses
- Loading branch information
Showing
4 changed files
with
11 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we want to automatically disallow opaque responses? It appears that
put()does not specifically disallow this, and so, you can usefetch()thenput()to overcome this limitation ofadd(). In any case, this should probably be allowed due to the rationale given under 6.4. Cross-Origin Resources and CORS which states that we want to allow "Caches to fetch and cache off-origin items."e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We changed add & addAll to reject on !ok responses, but we're not confident we can reveal that information on opaque responses.
We may reveal this in future, once we're happy with it security-wise.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use mode no-cors in
add(), you get aResponsethat is !ok. This effectively makes it impossible to cache items on a CDN usingadd(). However, you're able to work around this by usingfetch()thenput(). It sounds to me like the !ok text is overreaching perhaps since no-cors requests have !ok status?e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can cache items on a CDN using add if they have CORS headers. This is already required for fonts and JS modules.
We had two options:
"addAll caches responses as long as they're ok. It does not work for cross-origin no-cors"
Or
"addAll caches responses as long as they're ok. Unless they're cross-origin no-cors, in which case we'll cache them anyway, even though they may be HTTP 500 or whatever"
The latter seems much harder to explain, and failures may escape testing.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that cross-origin no-cors comes back with a status of 0 regardless (which is !ok). Why do we need to discern between statuses in that case?
In any case, why is this restriction not imposed on
put()and it is foradd()?e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakearchibald
FWIW I ran into this issue adding offline support to Lodash's website.
It was puzzling because docs like MDN Cache.put state
Or MDN Cache.add which states
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't a restriction, it's a feature. The feature is that addAll will reject if one or more of the responses is !ok. We made this change because this is what developers were expecting with this API.
With put, you pass in the response directly. You may have created it yourself, you may be creating a request-response mismatch. Since it's the lower level version, we assume you're checking the response yourself, and let you store whatever you want, even if it's a 404.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some APIs already expose !ok to some extent, such as AppCache and
<object>. If we figure out that's fully equivalent to exposingresponse.okon opaque responses, we will expose it. Then we can make addAll work with no-cors and still reject on 404s etc.e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdalton seems like MDN needs updating.
This was a potentially backwards incompatible change we (carefully) made following developer feedback. Understandable that MDN is out of sync.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jpmedley on updating the MDN content now that it is out of sync with the change post-feedback.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakearchibald
I donno...
It seems to like
fetch+cache.putis the way to go.The gist of the MDN description is correct. I did just...
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdalton throwing errors can be a feature. What we were doing before was arguably failing silently.
With
putyou put the response into the cache. If you can't see into the response, it's easier to understand that you're blindly caching it, and that comes with risks. It may be an HTTP 500 that will break all your demos.e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakearchibald
AFAIK it's the only way to do it. I couldn't use
cache.addbecause it failed straight away.That left
fetch+putas a way to get it working for things like cdns and other external resources.e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Hopefully those CDNs will start using CORS so you can reliably cache from them. They'll have to if they want to serve JS modules.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakearchibald
So is
fetch+putstill going to be an option? Or is the idea that service workers are intended to be broken/unusable for cdns/external resources until all use CORS?The appeal of APIs like
addwas that I could fetch() one or more requests, then add the result straight to the cache.e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdalton
Like I said in comments above,
fetch+putlets you store opaque responses today, and will continue to do so.addAllno longer does, because we added a feature so it fails on !ok responses like 404.addAllmay allow opaque responses in future, once the due diligence is done around security.e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be an option to add an "options" parameter to put/putAll/add/addAll to let websites decide for all these methods if they want !ok to be working or not? Might still be confusing if the default for different methods would be different...
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. The issues raised by Ali and myself weren't about using
addAll(it wasadd).Do your comments apply to
addtoo?e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkruisselbrink yeah, we discussed this. It's still possible, but I'd rather we just exposed "ok" on opaque responses.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdalton
add = request => addAll([request])e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakearchibald
Yep, yep. Why wasn't it
addAll => map(requests, add)?e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's an atomic operation. If one thing fails, nothing goes in, so it's more than a
Promise.all.We're going to explain this stuff better in future with cache transactions #823
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jakearchibald. To summarize:
add()is meant to be a reliable way to cache responses - on the other handput()does not have the same guarantees so you can put anyResponse(even with not ok status code e.g. cross-origin no-cors)add()is not the equivalent offetch()+put()add()for cross-origin no-cors in the futuree2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Allowing
okon opaque responses basically means proving the web is already broken in this regard 😀e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly the documentation could be improved, but we did try to update it. See:
https://bugzilla.mozilla.org/show_bug.cgi?id=1244764#c19
And the "Exception" section in these two pages:
https://developer.mozilla.org/en-US/docs/Web/API/Cache/add
https://developer.mozilla.org/en-US/docs/Web/API/Cache/addAll
Personally I'd be more inclined to remove
add()andaddAll()rather than try to make them magically work better for opaque responses. In hindsight I think it was a mistake to try to add a convenience fetch+put operation into the platform so soon.I also think people should probably avoid opaque responses in any kind of cache-first scenario. You just have no way to tell if the resource loaded or not. If you want to use a cache-first model you really need to get the resource served with CORS.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knows FB's API which returns OK when user not logged and not OK when logged. I don't think it makes sense to add another way to check this.
Well, you can add results straight to the cache with
put(), just write a little bit of a code.Basically, you just caught into
addAllcaching 404/500 responses. Imagine thataddAllwon't have this change and restriction and so you probably wouldn't even think that CDN may return an error and what it will be cached. But CDN will in some day, because of network problems, CDN down, etc.And then bad happens, your users get cached wrong response, website isn't working and ServiceWorker update doesn't help because you are probably caching static (CDN) resources in a separate cache which doesn't update (why would it, those assets are static after all). And then you probably don't know what to do, you don't know how to debug it and you are simply going to complain about ServiceWorker into the Twitter, this repo, etc.
Is that better? I don't think so.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details @jakearchibald!
I updated my caching technique (updated again with @wanderview's feedback) to attempt
corsfirst withfetch+put, then fallback tono-corswithfetch+putand things work. I found a Chrome bug to boot.e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdalton Just FYI, you are also running the risk of getting incoherently cached resources. It seems your site uses non-unique URL names with modest http cache max-ages. This means you could possibly pull some http cached resources and some non-http cached resources in your install event.
This is something @jeffposnick pointed out to me here:
https://twitter.com/jeffposnick/status/671451613224001538
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this example in MDN to reflect the
response.ok()check.Edit: Make that
response.ok. Computers are hard.e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, max-age on mutable content is usually a mistake https://jakearchibald.com/2016/caching-best-practices/
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wanderview
@jakearchibald
I believe it's a gh-pages thing.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdalton careful with using
.catch()for logging, it means you've handled the error. Promises are async representations of try/catch, so you've written something like:In the above, "It worked" will always be logged, since you handle the error. You probably want to re-throw the error if you're catching purely for logging.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, gh-pages doesn't let you change this (I call it out in the article, but doesn't look like they'll change).
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I know.
Naw. For my use offline caching is pure bonus. I don't care to handle the error, there's no dummy content or some such to fallback to, just having something logged is enough. From my earlier example you can see even with just logging there's plenty of indicators in the console.
e2a6d18There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, apologies if that was patronising - I've just seen that done in error a lot.