add `unwrap()` method to supabase client about supabase-js

[emschwartz]

Hi @kiwicopple I hear that and could accept that it's just a different style choice if the Promise were guaranteed never to reject (because then you wouldn't need to wrap it in a try/catch). However, this call to fetch can "reject on network failure or if anything prevented the request from completing" (MDN), which means you need to handle the error in two places instead of one. Always wrapping it in a try/catch and re-throwing the error returned seems worse than handling all types of errors in one way.

All of that said, I'm using your open source library so take my comments with a grain of salt :)

[mstade]

Proposed implementation available in supabase/postgrest-js#188.

[emschwartz]

They are? I'm familiar with async/await syntax but all of the examples here show data and error in the object that the Promise resolves to:

const { data, error } = await supabase
  .from('cities')

As as result, it seems like we need to write code like this:

try {
  const { data, error } = await supabase
    .from('cities')
  if (error) {
    throw error
  }
} catch (err) {
  // handle err
}

If the Promise were rejected instead of returning the error object, you could leave out the if (error) { throw error } bit.

(This is the line I would expect to throw instead of returning)

[mstade]

For what it's worth, the decision to return the error as a result rather than throw is described in #32. I opened a discussion about this some time ago asking pretty much the same thing as you, with a slightly (but not meaningfully) different take on it: supabase/supabase#604

We've found that we use both the { data, error } = supabase ... pattern and a variant of unwrap, so I'm not sure either way is strictly better and an .unwrap() modifier seems like a good way to go. What we do with our variant right now is (ironically) to wrap the query and throw if there's an error, like so:

try { 
  const data = await throwOnError(supabase ...)
} catch (error) {
  ...
}

I'm gonna take a stab at a PR for this, because it's annoying me to no end. 😅

[mstade]

I guess supabase/postgrest-js#188 doesn't technically fit the definition of the unwrap pattern described here, but more or less solves the problem anyway? Is it enough to close this issue, @andykais?

[andykais]

I would say it definitely covers my initial use case @mstade. Hats off to you! Thanks for putting in the time.

[emschwartz]

Oh whoops, @andykais sorry for the misunderstanding. My question was actually about the library as it currently stands rather than your suggestion.

I think your proposal seems like a reasonable way to improve the ergonomics without breaking backwards compatibility.

If there is going to be a major version change with breaking changes, I would vote for the pattern to be changed entirely so that all errors cause the Promise to be rejected.

[malobre]

I agree with @emschwartz, we end up try-catch'ing + checking for error in the returned object.
If the goal was to enhance the DX I think it should be harmonized (e.g catch thrown errors and put them in res.error).

Edit: The auth part already does this (e.g signUp)

[emschwartz]

Out of curiosity, why do all the calls return an error instead of rejecting the Promise? This seems like a very unusual pattern for Javascript/Typescript.

[andykais]

@emschwartz they are rejecting the promise. This is async/await syntax, which can use try/catch blocks to catch rejected promises

[andykais]

I understand your confusion now. If you look at my first message, the error is being thrown in the "Describe the solution you'd like" section. This is my suggestion on how an unwrap method should work. Above that I have an example of how the supabase client works currently, which is what you are describing.

[kiwicopple]

Hi @emschwartz - this is just a DX decision. We purposely chose to return errors rather than throw them. There is no right or wrong here, only preferences. We decided to return because (we believe) it is a nicer developer experience :) . I'm sorry you don't share the view - I didn't either until I started using it, so perhaps this library will change your mind

[Nick-Mazuk]

I do second this feature request.

Since a database call isn't usually the only thing that's happening, I've found that I've often needed to double catch errors. For instance:

try {
    const { data, error } = await supabase.from('table').select('*')
    if (error) throw new Error(error)

    // do other post-processing
} catch (error) {
    // handle the errors
}

This is a short example, but it can get especially cumbersome when combining this with other API calls for other services. Here's a contrived example that should hopefully get the point across.

try {
    const [{ data, error }, fetchResponse, otherApiResponse] = await Promise.all([
        supabase.from('table').select('*'),
        fetch('https://example.com/api-endpoint'),
        someOtherApiFunctionCall(),
    ])
    if (error) throw new Error(error)

    // do other post-processing
} catch (error) {
    // handle the errors
}

Notice how the Supabase error is handled differently from everything else?

Or what if you don't use async/await, and you use then/catch. For instance, inside of a react useEffect hook, then/catch is preferred since you cannot use async/await directly in hook.

const [posts, setPosts] = useState()
const [errorMessage, setErrorMessage] = useState('')

// example with no error handling
useEffect(() => {
    supabase
        .from('posts')
        .select('*')
        .then(({ data }) => setPosts(data))
}, [])

// with error handling
useEffect(() => {
    const getData = async () => {
        const { data, error } = await supabase.from('posts').select('*')
        if (error) setError(error)
        else setPosts(data)
    }
    getData()
}, [])

// error handling if supabase unwrapped promises
useEffect(() => {
    supabase
        .from('posts')
        .select('*')
        .unwrap()
        .then((data) => setPosts(data))
        .catch((error) => setError(error))
}, [])

I would argue that the unwrap option is the clearest option to read and write.

I understand that this is a matter of opinion and not objective fact, but when a lot of JavaScript is built around being able to reject promises, I find it hard to believe that not rejecting on errors is always better. At least having the option with unwrap I think is much better than the current schema.

[mstade]

Happy to help! 😊

[riderx]

OMG i just discovered that now, felt so annoyed by that as well, i tried to unwrap in SDK v2, but method seems not available how to use it in V2?

[riderx]

Ok found it .throwOnError()