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

fix: throwOnError function call should return response success type always #454

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dogukanakkaya
Copy link

@dogukanakkaya dogukanakkaya commented Jul 30, 2023

What kind of change does this PR introduce?

This PR improves the typing of .throwOnError function. Which should return PostgrestResponseSuccess<T> type.

What is the current behavior?

Right now .throwOnError is returning nullable data.

What is the new behavior?

If .throwOnError called on the query it will always return PostgrestResponseSuccess<T> instead of PostgrestResponseSuccess<T> | PostgrestResponseFailure

Additional context

This is a breaking change because previously you could call other functions after .throwOnError because it does not execute the query but only sets the shouldThrowOnError property to true to be used later on when executing the query.

With this change, chaining more functions after .throwOnError is not possible since it executes the query and check the result and throws in case of error or return the PostgrestResponseSuccess in case of success.

I saw this PR right after I finished working on that. Which resolves the same issue with Typescript only instead, which I also tried at first since it gives developers more flexibility since we don't necessarily have to execute the query after .throwOnError. But looks like that PR is freezed/stuck since Nov 2022 because of a Typescript issue.

Also related with: supabase/supabase-js#801

@wyozi
Copy link
Contributor

wyozi commented Aug 8, 2023

In my codebase I use a helper called dataOrThrow. It does what it says, i.e. do throwOnError but also change the return value to the .data property. It would be a potential option if avoiding breaking changes was a goal, although I think it would be a good addition to the codebase in the first place

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

Successfully merging this pull request may close these issues.

2 participants