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

Why doesn't fetch reject if 400 ≤ status < 600? #18

Closed
matthew-andrews opened this issue Dec 29, 2014 · 22 comments
Closed

Why doesn't fetch reject if 400 ≤ status < 600? #18

matthew-andrews opened this issue Dec 29, 2014 · 22 comments

Comments

@matthew-andrews
Copy link

It would be much more useful if it did, or if there were an option for it to.

@domenic
Copy link
Member

domenic commented Dec 29, 2014

There's a difference between an "error" and an "exception". Promise rejections parallel exceptions. But there's nothing exceptional about 4xx or 5xx errors on the web.

@matthew-andrews
Copy link
Author

What definition of error, exception and exceptional are you using? As far as I knew the ECMA script spec refers to “Error Exceptions” implying a certain degree of interchangeability to me. I've never come across exception's adjective form (exceptional) in a computer science context. Not sure what you mean by that.

My understanding (please correct me if I am wrong) was that an Error became an exception when it is “thrown” — and so in this case whether a 4xx or 5xx is exceptional on the web is decided by how they are dealt with, not by, say, how commonly they occur…

Do you have a reference for “Promise rejects parallel exceptions”? If you “throw an Error” (e.g. below) that is caught by a Promise.

fetch('./my-lovely-json-endpoint')
  .then(function(response) {
    if (response.status >= 400 && response.status < 600) {
      throw new Error("Bad response from server");
    }
    return response.json();
  })
  .then(function(data) {
    console.log("got some data", data);
  });

Very happy to be persuaded otherwise — and I can see how ignoring the status keeps fetch clean and simple — but all the fetch use cases I can think of would require this sort of 400 ≤ status < 600 check. I can't imagine fetch ever being a feasible alternative to an AJAX wrapper if you have to write this level of boilerplate to make a request for some json…

@dgraham
Copy link

dgraham commented Dec 29, 2014

@matthew-andrews Here's the wrapper we're using on github.com as we migrate from jQuery to window.fetch. At least the required wrapper is much smaller than the full jQuery AJAX stack.

@annevk
Copy link
Member

annevk commented Jan 1, 2015

Yeah, we could have a status filter of sorts I suppose. Question is how generic that should be.

@matthew-andrews
Copy link
Author

thinking about this more I really think that reject on 400+ statuses should have been the default behaviour, but it seems like that is a non starter. Offering it as an option, especially if that needs to be generic probably would be not a lot simpler than checking the status and throwing on >=400 manually.

@dgraham We've started using this to solve the problems we were having:-
https://github.com/matthew-andrews/fetchres

@annevk
Copy link
Member

annevk commented Jan 16, 2015

It's unclear to me why you think that makes sense. The body of a 400 or a 505 still carries semantic information that an application can use. And in fact <img> will happily display a 404 if that is an image.

@annevk
Copy link
Member

annevk commented Jan 16, 2015

Note that I did not say the option needs to be generic so I'm reopening this for now. We could have rejectHTTPErrors or some such.

@annevk annevk reopened this Jan 16, 2015
@matthew-andrews
Copy link
Author

superagent has quite an interesting alternative solution to this problem — it provides a res.ok that can be more easily checked against:-

 request
   .post('/api/pet')
   .send({ name: 'Manny', species: 'cat' })
   .set('X-API-Key', 'foobar')
   .set('Accept', 'application/json')
   .end(function(res){
     if (res.ok) {
       alert('yay got ' + JSON.stringify(res.body));
     } else {
       alert('Oh no! error ' + res.text);
     }
   });

Among other properties:-
https://github.com/visionmedia/superagent/blob/9e922cd34d70ead5ef4557c634a46153538134d5/lib/client.js#L385-409

@annevk
Copy link
Member

annevk commented Jan 17, 2015

That seems rather cool, although it does not have the nice property of rejecting. I could also imagine something like

fetch("...", {require:"2xx"})

@matthew-andrews
Copy link
Author

i like both of those approaches. require is probably already overloaded as a term but i can't think of a better suggestion.

@matthew-andrews
Copy link
Author

Been thinking about this a lot and I quite the TJ inspired solution:-

fetch('//mattandre.ws/path.json')
  .then(function(res) {
    if (res.ok) return res.json();
    return fallbackContent;
  });

I think it neatly addresses @domenic's concerns about client/server errors being unexceptional whilst adding just enough sugar to be convenient for developers to make handling these sorts of errors in a sensible way and it's backwards compatible*.

The nice property of rejecting is also limiting in a way because you might like instead of rejecting to do something else, say provide fallback content, instead.

What do you think?

* And can be easily polyfilled:-

(function() {
  var _fetch = fetch;
  fetch = function() {
    return _fetch.apply(this, arguments)
      .then(function(response) {
        response.ok = response.status < 400;
        return response;
      });
  };
}());

@annevk
Copy link
Member

annevk commented Jan 25, 2015

That polyfill seems incorrect. E.g. 399 should not match .ok, right? Or a 301 without a Location header?

@matthew-andrews
Copy link
Author

Ah probably. I just wrote that more to give one idea of how to hook ok in. The fact that the actual status check needs to be more complex just further shows the need developers have for a little more help from the fetch spec.

@annevk annevk closed this as completed in e62f57b Jan 27, 2015
@wanderview
Copy link
Member

Gecko bug to implement this: https://bugzilla.mozilla.org/show_bug.cgi?id=1126483

@tobireif
Copy link

tobireif commented Jun 3, 2015

I also ran into the same issue - 404s didn't cause errors, thus I wasn't able to catch() the issue and handle it. JakeChampion/fetch#155

"there's nothing exceptional about 4xx [...] errors on the web."

Fetching the file failed - that's a failure, no matter whether it's called exception or error, IMHO.

I venture to guess that there will be many more reports of people being surprised that fetch doesn't throw when there's a 404. And what's worse, there probably will be many 404s going unhandled, because many will expect fetch to throw on 404.

fetch (at least in FF https://bug1126483.bugzilla.mozilla.org/attachment.cgi?id=8555602 ) reports "response.ok" for status "204 No Content". For my current app that's not what I want, so I'll simply throw an error on any non-200. (When the status is 200 the content could still be zilch, but if it's certain that there's no content, I shouldn't resolve the promise I'm returning.)

function fetchStatusHandler(response) {
  if (response.status === 200) {
    return response;
  } else {
    throw new Error(response.statusText);
  }
}

and (in my case inside a larger Promise block):

fetch(url)
  .then(
    fetchStatusHandler
  ).then(function(response) {
    return response.blob();
  }).then(function(blob) {
    resolve(blob.size);
  }).catch(function(error) {
    reject(error);
  });

For the spec:

In order to prevent the user (who might expect fetch to throw on 404) from accidentally ignoring 404s etc, perhaps raise an error if there's no status code handler supplied by the user. (eg a required arg {statusHandler: func} .)

I also like the solution proposed earlier by Anne, eg with a different name:

fetch("...", {throwUnlessStatusIs: "20x"})

It should be a required arg so that the user knowingly accepts or rejects eg 404s.

@vaibsharma
Copy link

vaibsharma commented Mar 29, 2020

I prefer to do like this

return new Promise((resolve, reject) => {
        fetch(url, {
            method: POST,
            headers,
            body: JSON.stringify(body),
        })
            .then((res) => {
                const response = res;
                const responsePromise = res.json();
                responsePromise
                    .then((data) => {
                        if (response.status >= 400) {
                            reject(data);
                        } else {
                            resolve(data);
                        }
                    })
                    .catch((error) => {
                        reject(error);
                    });
            });
    });

@jakearchibald
Copy link
Collaborator

jakearchibald commented Mar 29, 2020

FWIW that can be rewritten as:

return fetch(url, {
  method: 'POST',
  headers,
  body: JSON.stringify(body),
}).then((response) =>
  response.json().then((data) => {
    if (!response.ok) {
      throw Error(data.err || 'HTTP error');
    }
    return data;
  }),
);

Also, you should only reject error objects (as I've done in the example above).

@annevk
Copy link
Member

annevk commented Mar 29, 2020

s/responsePromise.status >= 400/!response.ok/

@jakearchibald
Copy link
Collaborator

Ta, edited

@vaibsharma
Copy link

Thanks @jakearchibald

yutakahirano added a commit that referenced this issue Jun 23, 2020
# This is the 1st commit message:

# This is a combination of 23 commits.
# This is the 1st commit message:

Integrate CORP and COEP

This is part of the introduction of COEP
(whatwg/html#5454). The CORP check now takes
COEP into account. Also, responses coming from service workers
are checked.

# This is the commit message #2:

Update fetch.bs

Co-authored-by: Domenic Denicola <[email protected]>
# This is the commit message #3:

Update fetch.bs

Co-authored-by: Domenic Denicola <[email protected]>
# This is the commit message #4:

fix

# This is the commit message #5:

fix

# This is the commit message #6:

fix

# This is the commit message #7:

fix

# This is the commit message #8:

fix

# This is the commit message #9:

fix

# This is the commit message #10:

fix

# This is the commit message #11:

fix

# This is the commit message #12:

fix

# This is the commit message #13:

fix

# This is the commit message #14:

fix

# This is the commit message #15:

fix

# This is the commit message #16:

fix

# This is the commit message #17:

fix

# This is the commit message #18:

Update fetch.bs

Co-authored-by: Anne van Kesteren <[email protected]>
# This is the commit message #19:

Update fetch.bs

Co-authored-by: Anne van Kesteren <[email protected]>
# This is the commit message #20:

fix

# This is the commit message #21:

fix

# This is the commit message #22:

fix

# This is the commit message #23:

fix

# This is the commit message #2:

fix
@hugo-dlb
Copy link

hugo-dlb commented Nov 12, 2020

A good workaround is to do a wrapper around fetch like an HttpClient class:

export default class HttpClient {

    static post(url: string, body: any): Promise<any> {
        return new Promise(async (resolve, reject) => {
            try {
                let result = await fetch(`${process.env.REACT_APP_API_BASE_URL}/${url}`, {
                    method: 'POST',
                    headers: {
                        'Content-Type': 'application/json'
                    },
                    body: JSON.stringify(body)
                });
                if (result.status >= 400) {
                    reject(await result.json());
                } else {
                    resolve(await result.json());
                }
            } catch (err) {
                reject(err);
            }
        });
    }
}

Then it can be used like this...

HttpClient.post('/auth/login', {
        email,
        password
    }).then(res => {
        // Success
    }).catch(err => {
        // Error
    });

@marcelo-ribeiro
Copy link

marcelo-ribeiro commented Jul 30, 2022

// Typescript
export const fetcher = async <T = any>(
  input: RequestInfo,
  init?: RequestInit
): Promise<T> => {
  const response = await fetch(input, init);
  if (!response.ok) throw new Error(response.statusText);
  return response.json();
};

// Javascript
export const fetcher = async(input, init = {}) => {
  const response = await fetch(input, init);
  if (!response.ok) throw new Error(response.statusText);
  return response.json();
};

Usage example:

type UserPost = {
  userId: number;
  id: number;
  title: string;
  body: string;
};

(async () => {
  try {
    const data = await fetcher<UserPost>(
      "https://jsonplaceholder.typicode.com/posts/1"
    );
    const { userId, id, title, body } = data;
    console.log({ userId, id, title, body });
  } catch (error) {
    console.log(error);
  }
})();

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

10 participants