Skip to content
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

Option to require response.ok #103

Closed
jakearchibald opened this issue Aug 5, 2015 · 22 comments
Closed

Option to require response.ok #103

jakearchibald opened this issue Aug 5, 2015 · 22 comments

Comments

@jakearchibald
Copy link
Collaborator

Something like:

fetch(url, {
  requiredStatus: 'ok'
})

…which would cause fetch to reject if response.ok is false. Default value would be any. It could be a boolean, but a value like this means we could let the developer require a specific status code.

This would always reject with no-cors requests to avoid x-origin leakage.

@jakearchibald
Copy link
Collaborator Author

This discussion started at w3c/ServiceWorker#407

@wanderview
Copy link
Member

Would this be added to Cache add/addAll as well?

@jakearchibald
Copy link
Collaborator Author

I think…

cache.addAll([
  '/hello/',
  '/world/'
].map(u => new Request(url, {requiredStatus: 'ok'})));

…would be enough

@domenic
Copy link
Member

domenic commented Aug 5, 2015

I don't think this sugar is worth it. { requiredStatus: ok } saves very little characters over .then(res => { if (!res.ok) throw new TypeError() }), even with an informative message. And as you note, it spreads to more and more of the API.

People just need to realize the underlying mental model is that fetch returns responses, and responses can be not-OK, and they should handle that.

@jakearchibald
Copy link
Collaborator Author

It's a little more than that:

installEvent.waitUntil(
  caches.open('static-v1').then(cache => {
    return Promise.all([
      '/hello/',
      '/world/'
    ].map(url => {
      let request = new Request(url);
      return fetch(request).then(response => {
        if (!response.ok) throw Error("NOT OK");
        return cache.put(request, response);
      });
    }));
  })
);

// vs…

installEvent.waitUntil(
  caches.open('static-v1').then(cache => {
    cache.addAll([
      '/hello/',
      '/world/'
    ].map(u => new Request(u, {requiredStatus: 'ok'})));
  })
);

Also, the former isn't atomic. This could just be an option in cache.addAll of course.

@wanderview
Copy link
Member

What about adding the option to CacheStorage.open() so the entire Cache object could be marked requiredStatus 'ok'?

@annevk
Copy link
Member

annevk commented Aug 6, 2015

IMO, since Request is used for both cache and network operations, we should try to keep their semantics in sync, even when it's a somewhat trivial feature for either network or cache.

@jakearchibald
Copy link
Collaborator Author

Media elements, <track>, appcache and importScripts all fail to use body content if the status code looks like an error. <link>, <object>, <script> fails on 404, but it's not clear what they do with other error codes, but it seems like the intention is for them to consider other error codes to also be a failure.

Feels like there's call for this to be part of fetch to standardise what's considered and error and what isn't.

@annevk
Copy link
Member

annevk commented Aug 10, 2015

I know <img> always succeeds, but I guess we'd need to study the various call sites indeed. Ideally if they have restrictions on the status code it's only requiring "ok" and not something more complicated. Though I suspect that <track> and such will challenge the user for a 401 first, before failing...

@jakearchibald
Copy link
Collaborator Author

I guess 401 handling should come before the 'ok' check if there's a "window" set.

@annevk
Copy link
Member

annevk commented Aug 10, 2015

Yeah, I guess the "ok" check would be just before returning the response to the caller in main fetch.

@jakearchibald
Copy link
Collaborator Author

Another data point from https://w3c.github.io/network-error-logging/#h-reporting:

The user agent may classify and report server error responses (5xx status code, [RFC7231]) as network errors

The definition of what constitutes a network failure would remove the ambiguity here. But then, that spec doesn't count mixed content & CORS as network failures, which seems odd to me.

@annevk
Copy link
Member

annevk commented Aug 10, 2015

That specification seems to lack proper integration in the appropriate layers.

@annevk
Copy link
Member

annevk commented Aug 10, 2015

It would be great for this issue to verify the implementations of <script>, <track>, and <link>. And others, if feasible. So that we don't design a primitive that is not actually in use.

@domenic
Copy link
Member

domenic commented Aug 10, 2015

Haven't we already designed the primitive? It's response.ok.

@annevk
Copy link
Member

annevk commented Aug 10, 2015

@domenic it doesn't help if you ignore everything from #103 (comment) onward or pretend it's not meaningful.

@domenic
Copy link
Member

domenic commented Aug 10, 2015

I'm not. Everything from then onward seems to be talking about breaking down responses into two categories: those that are OK and those that are not OK. That's the primitive. Everything on top of that is sugar.

@annevk
Copy link
Member

annevk commented Aug 10, 2015

No, e.g., @jakearchibald pointed out not having this would not allow for atomic cache operations.

@domenic
Copy link
Member

domenic commented Aug 10, 2015

I think this is coming down to different definitions of primitive, which is what I found confusing. (E.g. a primitive would not be "perform atomic cache operations based on the OK-ness". You would combine the two primtives, "mutex" and "OK-ness", to be able to do atomic operations on OK-ness or on any other thing.) At this point I guess there's nothing further to be clarified so I'll bow out.

@jakearchibald
Copy link
Collaborator Author

Developer request for this http://tjvantoll.com/2015/09/13/fetch-and-errors/

@tjvantoll
Copy link

Developer request for this http://tjvantoll.com/2015/09/13/fetch-and-errors/

Well, I was trying to show that the default behavior of fetch() went against my expectations, because it differs from the behavior of $.ajax in jQuery and $http in Angular. I'm not sure a flag is going to help here because the default behavior won't change, which is the thing that threw me off.

Honestly I don't think this is that big of a deal, as after 5 minutes of Googling I knew what I needed to do, and I know how to use the fetch() in the future. I wrote the article to help get the word out because I'm confident I won't be the first person to run into this. (I quickly polled a few friends and colleagues, and every one of them expected a 500 response to go into catch() and not then().)

@annevk
Copy link
Member

annevk commented Nov 2, 2015

Having thought about this a bit more I agree with @domenic that the Cache API would need to expose that other primitive. That way you can e.g., require a certain header or a specific status.

I don't think we should add anything special to either the Cache API or Request for this particular case and I also hope we continue to avoid divergence between the Cache API and Request objects.

@annevk annevk closed this as completed Nov 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants