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

Add Fetch Assertions #1683

Open
shgysk8zer0 opened this issue Jul 3, 2023 · 1 comment
Open

Add Fetch Assertions #1683

shgysk8zer0 opened this issue Jul 3, 2023 · 1 comment

Comments

@shgysk8zer0
Copy link

As an extension and counter to #1679 I propose adding fetch assertions.

Example

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

Reasoning

  • Many languages/libraries regard 4xx & 5xx statuses as errors (error statuses)
  • Many devs (esp inexperienced) assume fetch() throws under conditions in which it does not
  • There is precedent for throwing under certain conditions (eg signal or integrity)
  • We can already assume errors based on the mismatch of the Accept & Content-Type headers (here the matching is acceptable)
  • This potentially allows for arbitrary error conditions
  • This is backwards compatible, though checking status and headers where not supported kinda defeats the purpose (where not supported, assert it's ignored and checks still have to be made, but it's a single thing to polyfill)
  • It can be extended to include other assertions in a backwards-compatible way
  • It allows for more meaningful errors
  • It can close a connection early in the event that an assertion fails (a potential maxContentLength comes to mind, probably as an additional option)
  • taking integrity as example (which is probably duplicated in functionality here), this provides additional error conditions where the response isn't what the dev expected
  • It provides for a new type of error (maybe HTTPError or FetchAssertionError) without conflicting with current usage
  • In the case of maxContentLength, this could be used to avoid excessive data usage on slow/mobile connections
  • In general, there are many ways in which we should know early that fetch() results in an error based solely on the response + headers but can't currently throw and close
  • It does not preclude a new error type which has access to response status + headers + body, which may be important to certain errors (e.g. a 500 "Internal Server Error" that often responds with HTML instead of JSON and is the actually relevant error)
@jakearchibald
Copy link
Collaborator

  • Many devs (esp inexperienced) assume fetch() throws under conditions in which it does not

That means developers need to learn to look at the response before assuming it's ok, or they need to learn to correctly use your proposed configuration options. Those feel of similar complexity.

The current system of checking response.ok means you can use the response if it isn't ok. Whereas that isn't the case with this proposal.

The acceptable option could be split out as response.assertAcceptable(), which is more composable as you can check response.ok and response.assertAcceptable() separately, and react differently to each case.

However, "does the server say the response is JSON?" seems much less useful than "does the response parse as JSON?". If I'm expecting JSON, having a response that parses as JSON seems like a stronger signal that I'm getting what I expect than taking the server's word for it.

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

2 participants