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

Implement unwrap() to throw query errors instead of returning them #188

Merged
merged 2 commits into from Jun 11, 2021
Merged

Implement unwrap() to throw query errors instead of returning them #188

merged 2 commits into from Jun 11, 2021

Conversation

mstade
Copy link
Contributor

@mstade mstade commented Jun 4, 2021

What kind of change does this PR introduce?

This adds an unwrap() function on the PostgrestBuilder class, which means errors will be thrown instead of returned.

Basic tests were added to test/basic.ts and the unwrap function is documented to some degree at least, though this can probably be improved. This should be a non-breaking change as it just adds functionality, it doesn't change any existing features.

What is the current behavior?

Supabase queries return errors as part of a successful response, i.e. the promise is resolved. The exception (no pun intended) to this is if there's something wrong with the fetch call itself, such as a network error – these will lead to the promise rejecting and can be handled in a try/catch block. This leads to somewhat awkward code:

try {
  const { data, error } = await supabase.from('my_table').select()

  if (error) {
    // This means there was an error with the query, we have to rethrow in order to handle it all below
  }
} catch (e) {
  // This means there was an error with the actual fetching, e.g. network issues
}

What is the new behavior?

By adding unwrap() to the query, we instruct the client to throw errors instead of returning them, meaning we can simplify the above:

try {
  const { data } = await supabase.from('my_table').select().unwrap()
} catch (e) {
  // This catches all errors, be they network or query related
}

Additional context

The rationale and motivation for this change is well described in supabase/supabase-js#92 I think, and also there's some rambling from me around this in supabase/supabase#604.

This implementation doesn't actually unwrap the result as described in supabase/supabase-js#92, i.e. returning only the actual data. This is done deliberately for two reasons:

  1. Making sure the caller can still extract count, status and statusText while still benefitting from errors being thrown instead of returned
  2. Ease the transition from the non-throwing pattern where destructuring is norm anyway, so less code has to be rewritten to take advantage of unwrap

It may be worth considering normalizing the network errors in a new major version of the client, so that network errors are also by default returned as part of a resolved promise. That way, you'd have consistent error handling no matter what the issue was – either you use the { data, error } = ... pattern regardless of error type, or you tack on unwrap() and all errors get thrown. I think that's out of scope for this PR though.

The rationale and motivation for this change is well described in supabase/supabase-js#92,
and further supported by the discussion in supabase/supabase#604.

This implementation doesn't actually unwrap the result as described in supabase/supabase-js#92,
i.e. returning only the actual data. This is done deliberately for two reasons:

1. Making sure the caller can still extract `count`, `status` and
`statusText` while still benefitting from errors being thrown instead of
returned
2. Ease the transition from the non-throwing pattern where destructuring is
norm anyway, so less code has to be rewritten to take advantage of unwrap

Basic tests were added to test/basic.ts and the unwrap function is
documented to some degree at least, though this can probably be improved.
@andykais
Copy link
Contributor

andykais commented Jun 8, 2021

const { data } = await supabase.from('my_table').select().unwrap()

fwiw, an unwrap pattern doesnt need a { data: T, error: Error } signature, it just needs T (because the error can be thrown). So typical unwrap usage would look like:

const data = await supabase.from('my_table').select().unwrap()

it also helps be explicit that an error will be thrown when using unwrap

@mstade
Copy link
Contributor Author

mstade commented Jun 8, 2021

Thanks for taking a look @andykais and for your comments! You're absolutely right, unwrap is probably a misnomer for this API. As I wrote in Additional context above I deliberately made it so it doesn't actually unwrap the data property, because there are actually more details than just data that gets returned with a successful response, count for example.

Certainly it wouldn't be particularly difficult to actually unwrap the data, but in that case it wouldn't be possible to get count etc. without going back to the { data, error } pattern. Maybe that's ok, but I figured it'd be annoying to have to go back to try + if to catch errors just to also get count etc.

Would it make more sense to call the API something other than unwrap perhaps? Just as you say, it isn't actually unwrapping the result, really it's just making sure to throw any errors coming from the fetch call (that isn't a network error etc.) so maybe it'd make more sense to rename it to throwErrors or something like that?

@andykais
Copy link
Contributor

andykais commented Jun 9, 2021

@mstade that sounds reasonable to me. I wasn't aware of other fields like count. I think you could still refer to it as unwrap and not confuse users if we keep the existing structure but pop off the error field. It will never be populated anyways, so if it is not present, that will be a good indicator to typescript users that the error is being thrown

@soedirgo
Copy link
Member

LGTM, thanks for the PR @mstade!

It may be worth considering normalizing the network errors in a new major version of the client, so that network errors are also by default returned as part of a resolved promise

That's right, we're working on supabase/supabase-js#170 where non-unwrapped calls should catch all errors, including network errors from fetch.

unwrap is probably a misnomer for this API

Agreed, this is different from e.g. unwrap in Rust or left in Haskell. Maybe throwOnError would be a better name?

@mstade
Copy link
Contributor Author

mstade commented Jun 10, 2021

Thanks for the review and comments @soedirgo! 🙏

I went with rejectErrors in supabase/supabase#604 to keep with Promise terminology, but your throwOnError alternative seems like a better suggestion to me. Does what it says on the tin, that's always a good thing I reckon! I'll make the change and push later on this evening. Stay tuned!

The `unwrap` moniker was a bit of a misnomer, since it didn't actually
unwrap the result, and this should make things a bit more clear. This was
discussed in a bit more detail in #188.
@mstade
Copy link
Contributor Author

mstade commented Jun 11, 2021

My apologies for the delay @soedirgo – life got in the way! Anyway, I've pushed the name changed in 3a61484 and also updated the tests and snapshots to reflect the change. Happy to make any further changes should there be a need! As I'm sure you can tell I'm no TypeScript expert, so I'm not sure whether there's a need to change the type signatures, so your guidance is much appreciated!

@soedirgo
Copy link
Member

All good—thanks again for the PR! :)

@soedirgo soedirgo merged commit 9df1e84 into supabase:master Jun 11, 2021
@mstade
Copy link
Contributor Author

mstade commented Jun 11, 2021

My pleasure, sir! 😊

@github-actions
Copy link

🎉 This PR is included in version 0.31.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@logemann
Copy link

logemann commented Jan 18, 2022

@mstade thanks for the super nice PR. As a 15+ yrs java developer, it really hurt not having the API rejecting the promise on error by default but your backwards compatible fix made my day! Just cant live w/o my try/catch haha.

@mstade
Copy link
Contributor Author

mstade commented Jan 18, 2022

@logemann no worries chief – thanks for the kind words! 🙏🥰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants