Consider response.throwIfNotOk()

[annevk]

Python has https://requests.readthedocs.io/en/latest/_modules/requests/models/#Response.raise_for_status and it seems folks sometimes want something like that from fetch(). You can branch on response.ok already, but perhaps essentially asserting would be nicer?

(They really seem to expect it to reject automatically, but that's not something that makes sense for all callers.)

(Naming follows https://dom.spec.whatwg.org/#dom-abortsignal-throwifaborted.)

[jakearchibald]

In terms of naming, response.assertOk() might make more sense, to avoid the 'not'.

I think the trick here is to make the error message useful to reporting systems. I know error text isn't usually spec'd, but it seems like the error message should include the status and statusText.

[justinfagnani]

Another option is to add a new option to fetch():

fetch(url, { throwIfNotOk: true };

Though response.assertOk() is usable in more than just fetch.

[shgysk8zer0]

I'm supportive of this, but an additional method to call won't address the problem of devs assuming that fetch() throws for them. Having something like assertOk added to the options would be better, except we'd still want access to the response here. I don't think that throwing a TypeError is ideal here.

However, to preserve similarity to throwIfAborted() without a "Not", I suggest throwIfError() or throwIfErrorStatus(). 4xx and 5xx are client/server error statuses, so I think it works.

[swantzter]

I came into this agreeing with @shgysk8zer0 that throwing a "plain" TypeError or Error might not be ideal. For a second I was considering if it would make sense to put the Resonse as cause on an Error, since the error is caused by, well, the response. But on second thought that would seem to go against the idea of cause.

But the more I think about it just an error with status and statusText feels sufficient like @jakearchibald suggested - for any more conplex needs its limits might provide a good push towards branching on response.ok, and that is what I'd personally reach towards in almost all cases even if a function asserting the status existed

[shgysk8zer0]

If you get a response with a status, it's pretty likely that the response body contains the actual error, and the headers contain other useful info. And, unlike other fetch() errors, we will have a response with headers and maybe a body here. I think that warrants different behavior.

Consider HTTP 404 Method Not Allowed for example. The Allow header is required for that status, so throwing an error that loses the headers is just not very useful. To omit that info would be like giving a 302 Found status but not the Location header... It's incomplete, at best.

I strongly advocate that any such method that throws an error only upon receiving a response provides access to the body and the headers. It really does make the difference between being nearly useless and actually useful. Plus, we already have the response (completely, outside of streaming)... We may as well make use of it instead of just throwing it away. This probably implies the addition of an HTTPError or ResponseError.

Further, the suggestion is modeled to follow throwIfAborted(), and AbortSignal also has the aborted (sorta the inverse of ok) and the reason properties. reason is determined by what's passed to controller.abort(). If this is to introduce a resp.throwIf*/resp.assertOk() method, it should also introduce at least a resp.error, where the new method is basically equivalent to:

Response.prototype.throwIfError = function() {
  if (! this.ok) {
    throw this.error;
  }
};

[domenic]

It sounds like people want access to the response. In that case, I don't think this method is useful, as you already have access to the response before calling the method.

In other words, the difference between

const res = await fetch(url);

try {
  await res.throwIfNotOk();
  
  // use res
} catch (e) {
  // use e.res
}

and

const res = await fetch(url);

if (res.ok) {
  // use res
} else {
  // use res
}

doesn't seem like a win to me.

The only case in which this method would be a win is if you're treating network errors (no response) and not-OK errors uniformly; then you could just write

const res = (await fetch(url)).throwIfNotOk();

// use res

where you assume any errors bubble up to some central error-handling mechanism which doesn't care what case you're in. But it sounds like that's not realistic in actual apps, so, we shouldn't add this.

[justinfagnani]

@domenic I think it is relatively common that you want to treat not-OK errors as exceptions. This is quite often the rationale for people still using libraries axios over fetch, and I've unfortunately seen a lot of code like:

let data = await (await fetch(url)).text();

It's also why some libraries like React Query specially call out fetch() as needing the author to throw errors: https://tanstack.com/query/v4/docs/react/guides/query-functions#usage-with-fetch-and-other-clients-that-do-not-throw-by-default

We have this in Lit Task too, where we differentiate successful task runs from errored run, and to make a task from fetch you usually need to make it throw to designate 4xx, 5xx, etc. error.

I think one of the big benefits from a more ergonomic API is in education. If throwing on no-OK responses is a single option or one-liner, more examples will include it, and more developers reading those examples will learn that errors need handling whether they use the assertion method or branch on status.

[jakearchibald]

@domenic

The only case in which this method would be a win is if you're treating network errors (no response) and not-OK errors uniformly

Totally agree. I tend to write something like this:

async function getData(url) {
  const response = await fetch(url);

  try {
    const data = await response.json();
    // Use presumably more accurate error data from the response:
    if (data.error) throw Error(data.error);
    return data;
  } catch (error) {
    // Error seems like a JSON parsing issue:
    if (response.ok) throw error;
    // Construct an error from the status:
    throw Error(`Response error: ${response.status} - ${response.statusText}`);
  }
}

throwIfNotOk doesn't really help here.

[shgysk8zer0]

I think that what I have in mind is better suited for its own issue upon thinking about it, but I do want to push back a little against some assumptions being made here:

• there are other reasons for fetch() to error (CORS, CSP {redirect:'error'}, an aborted signal, so it's not a given that fetch() is taking place outside of a try
• fetch().then().catch() should still be taken into consideration
• Not all responses will be JSON - it may be that an internal server error is HTML when the expected response is JSON (resp.json() would just throw a not-so-useful JSON parsing error there)

What I have in mind here would better fall under "Fetch Assertions". For the sake of addressing the issue at hand here, that'd be:

try {
  const resp = await fetch(url, {
    headers: { Accept: 'application/json' },
    assert: { ok: true, acceptable: true },
    redirect: 'error',
    mode: 'cors',
    integrity: '... ',
    signal: AbortSignal.timeout(3000),
    /* ...*/
  });
  // Handle response knowing it's `ok` and JSON per the Accept/Content-Type headers
} catch (err) {
  // Handle error
}

I think that's the more versatile and useful approach. This would leave open any number of error conditions a dev could specify based on request and response headers. And with an additional option could close the connection early (maybe based on Content-Type or Content-Length).

I still think that the actual response body and headers will be highly relevant to many errors thrown, but let's treat that as a separate issue for now.

My proposal is adding fetch assertions, with status/statusRange/ok being among the options. If assertions are not met by the response headers/status, throw an error (whether or not that is a new type of error with response headers and body is a different question).

[shgysk8zer0]

See #1683

[jimmywarting]

I think it's kind of common to see ppl doing something in the lines of:

async function get() {
  const res = await fetch(request)
  if (!res.ok) throw new Error('not okey status code.')
  return res.json()
}

but this will leave the request hanging and still consume bandwidth until it downloads everything?
And lower the max concurrent request being made at one single point?

if they would have done it more correctly then it would have been something like:

async function get() {
  const ctrl = new AbortController()
  const res = await fetch(url, { signal: ctrl.siganl })
  if (!res.ok) ctrl.abort(new Error('not okey status code.'))
  return res.json()
}

This would abort the request faster if it was not a okey response.

So maybe an assert option would be useful by helping the developer doing the "correct" thing by aborting the request prematurely if the response is not okey? if by that means doing the most sensable "correct" thing.

[justinfagnani]

@jakearchibald

Totally agree. I tend to write something like this:

async function getData(url) {
  const response = await fetch(url);

  try {
    const data = await response.json();
    // Use presumably more accurate error data from the response:
    if (data.error) throw Error(data.error);
    return data;
  } catch (error) {
    // Error seems like a JSON parsing issue:
    if (response.ok) throw error;
    // Construct an error from the status:
    throw Error(`Response error: ${response.status} - ${response.statusText}`);
  }
}

throwIfNotOk doesn't really help here.

Doesn't it a bit though?

async function getData(url) {
  const response = await fetch(url);
  response.throwIfNotOk();
  const data = await response.json();
  // Use presumably more accurate error data from the response:
  if (data.error) throw Error(data.error);
  return data;
}