fix #20: invoke does not throw on HTTP Error status codes

[vejja]

What kind of change does this PR introduce?

invoke() call throws on HTTP errors if shouldThrowOnError flag is set
Fixes #20

What is the current behavior?

invoke() does not report HTTP error status codes.
In that case it returns a clean { data, error: null } object with the 'data' property populated, and the HTTP status code is lost.

What is the new behavior?

invoke() checks if HTTP status code is other than 2xx, then reports this as an error with the HTTP status code populated.
In addition, if the shouldThrowOnError flag is set, invoke() will throw rather than returning the {data, error} object.

This is more in line with the behavior of similar libraries (e.g. Firebase).

Additional context

The proposed PR also exports an HttpError class that can be used

• to check whether the error thrown by the client is arising from a faulty HTTP status code vs. other possible errors (e.g. connection errors etc.). The check can be written as if (error instanceof HttpError) {}
• to extract and process upon the faulty HTTP status code with if (error.statusCode === xxx) {}
• to inspect the root cause of the error in case the responding serverless edge function returned the original error context. This is being forwarded by HttpError in its error.data property

[jaitaiwan]

I did a small review of this PR because it is partially addressing a current issue I have with invoke's implementation. Prior to this PR the error object would not have an error if the error was created by running new Response(..., {status:455}) for example. So I think that based on the original issue which I think somewhat mentions this, we could/should ensure that when shouldThrowOnError is not set that the error object still contains the http error.

[vejja]

I did a small review of this PR because it is partially addressing a current issue I have with invoke's implementation. Prior to this PR the error object would not have an error if the error was created by running new Response(..., {status:455}) for example. So I think that based on the original issue which I think somewhat mentions this, we could/should ensure that when shouldThrowOnError is not set that the error object still contains the http error.

Hi @jaitaiwan
This PR ensures that.
Even if shouldThrowOnError is not set, the error object will still contain the properties ‘statusCode’, ‘statusText’ and ‘data’.
You can check that in the catch section of the code.
Hope this answers your request, if not let me know
Best !

[soedirgo]

Thanks for the PR!

I think making the error a class is preferred, but can we make it only for the error itself and have status, statusText as separate fields? i.e. like this, but with FunctionsError class instead of PostgrestError type.

[soedirgo]

I think @jaitaiwan's point is that even if shouldThrowOnError: true, data should be null and the error message should be set to the await response.text() instead of 'Invoke call returned HTTP Error code'

[vejja]

Thanks for the PR!

I think making the error a class is preferred, but can we make it only for the error itself and have status, statusText as separate fields? i.e. like this, but with FunctionsError class instead of PostgrestError type.

@soedirgo
The PostgrestError type is defined as

export type PostgrestError = {
message: string
details: string
hint: string
code: string
}


However the edge server returns new Response(..., {status, headers}), where '...' can be anything from a text string, a JSON object or even a Blob.

In other words we have no guarantee that the edge server will return a JSON object with well-formatted properties such as {message, details, hint, code}, is my understanding correct ?
If it is the case, there is no benefit to return a FunctionsError class ?

[vejja]

I think @jaitaiwan's point is that even if shouldThrowOnError: true, data should be null and the error message should be set to the await response.text() instead of 'Invoke call returned HTTP Error code'

@soedirgo
Say for instance the edge server returns new Response({message, details, hint, code}, {status = 403})
In that case on the client side, the {message, details, hint, code} object would be properly parsed by await response.json(), but would be flattened to string with await response.text()

[soedirgo]

OK, I think I got confused, so ignore what I said.

From what I gather, @jaitaiwan's comments agree with the PR - if the edge function responds with 4xx/5xx, the response should be in error, not data.

So the PR makes sense, but some improvements we can make:

• pull statusCode & statusText out into { data, error, status, statusText } - we might also want these for successful requests as well
• status and statusText should be optional because these are not set in the case of FetchError, relay error, JSON.parse error, etc.
• rename HttpError to FunctionsError to clarify that the error comes from an edge function - HttpError makes it look like a generic HTTP client error.

Does that look OK?

[vejja]

So I think the questions we are trying to answer here is :

Can we assume that, in case of HTTP status code other than 2xx, the Response of the edge server will always be a JSON object with headers ”Content-Type”:”application/json” and properties {message, details, hint, code} ?

FYI Firebase answered “Yes” to this question by doing 2 things:

1. Server-side, by returning non-2xx HTTP status codes by way of throwing HttpsError
2. Client-side, by throwing FunctionsError

Now, in order to do this, Firebase enforces HttpsCallable to send JSON requests and receive JSON responses.

As far as Supabase is concerned though, we can send requests of any type and receive responses of any type.

So one solution would be the following

• have the generic invoke() call which leaves it up to the developer to decide what “content-type” to send and to receive. invoke() would return a response object in the form of {data, error, status, statusText} and in case of error, the error object would be populated with whatever content was returned from the server. It would be up to the developer to parse the error property according to his needs
• Have a specific invokeJson() call which enforces ”application/json” on the request and assumes the same on the server response. The response object would also be returned in the same form of {data, error, status, statusText} but here we would have 2 additional guarantees: (1) that data is a JSON object, and (2) that error is an instance of FunctionsError with properties {message, details, hint, code}

What do you guys think ?

[vejja]

OK, I think I got confused, so ignore what I said.

From what I gather, @jaitaiwan's comments agree with the PR - if the edge function responds with 4xx/5xx, the response should be in error, not data.

So the PR makes sense, but some improvements we can make:

• pull statusCode & statusText out into { data, error, status, statusText } - we might also want these for successful requests as well
• status and statusText should be optional because these are not set in the case of FetchError, relay error, JSON.parse error, etc.
• rename HttpError to FunctionsError to clarify that the error comes from an edge function - HttpError makes it look like a generic HTTP client error.

Does that look OK?

Ok looks good - and sorry I sent my comments simultaneously

Now there are 2 small things I want to confirm:
1- By pulling out status and statusText out of error, we are losing this information when we catch the error on the client side. (But then the server-side can provide details in its response)
2- FunctionError is now the raw Response.data of the server. So it could be a Blob, and hence cannot inherit from Error anymore. It can be anything really, and it has no pre-determined property fields (but then again it can be the responsibility of the developer to know the response type in advance and deal with it)

is my understanding correct ?

[soedirgo]

Agree on 1.

On 2, FunctionsError would be exactly the same as your HttpError implementation here, just with a different name. So FunctionsError can inherit Error, and it just has one extra data field. error.data would depend on the responseType, similar to data for successful responses.

For TypeScript use, you'd use error instanceof FunctionsError to check if it's an edge function error vs. FetchError, relay error, etc.

If you need different response types for 2xx vs. 4xx/5xx responses, you'd need to do responseType: 'blob' and then process the blob conditionally.

[vejja]

Agree on 1.

On 2, FunctionsError would be exactly the same as your HttpError implementation here, just with a different name. So FunctionsError can inherit Error, and it just has one extra data field. error.data would depend on the responseType, similar to data for successful responses.

Ok so additional questions here

• what Error.message do you want in the constructor ? I initially chose a verbose message that said that the edge function had returned a non-2xx code but it could be a better sentence, or even the status code (even if it does not feel right)
• If we were running the compiler for ES2022, we could benefit from the new syntax of Error(message, {cause}) and we could use the ´cause ´ option instead of adding a ‘data’ property. Any objection to upgrading to ES2022 in tsconfig ?

[soedirgo]

1. Not too opinionated here, also agree verbose is better. Maybe s/HTTP error/non-2xx status?
2. Looking at caniuse, requiring 2021+ might be prohibitive, and IIRC this is only available for Node.js 16+. Also, I believe the cause would also need to extend Error.

[vejja]

• Not too opinionated here, also agree verbose is better. Maybe s/HTTP error/non-2xx status?
• Looking at caniuse, requiring 2021+ might be prohibitive, and IIRC this is only available for Node.js 16+. Also, I believe the cause would also need to extend Error.

Sorry wasn’t clear here on ES2022
I meant allowing TypeScript to use 2022, but still compiling down to target ES2015

[soedirgo]

Hmm not sure if that would work: link. Honestly I'm not too familiar with how tsc "downlevels" the newer JS features.

There's also still the issue of cause needing to be an Error.

[vejja]

OK you are right, then I think everything clear on my side
Will change the PR now

[vejja]

@soedirgo Just pushed the refactor.

I realized that we are now in a weird situation where

• if everything went ok, we are returning status and statusText - which in that case are probably a 200 OK that isn't worth actioning anything upon.
• if something went wrong and the shouldThrowOnError flag is set, we are losing the status and statusText, which are probably worthy examining.

On balance I believe this is also the way it works for the PostgrestClient so maybe we don't care. The only difference is that in case of error, PostgrestClient always returns a JSON body with a well-formatted {message, hint, code, details} object.

[thorwebdev]

Hey @vejja, thanks so much for this PR! We've discussed this a bit more internally and are actually contemplating moving the behaviour of invoke closer to the behaviour of fetch. So rather than processing the content type within invoke, we'd return { response, error } where response is actually the response returned from fetch. Then you'd have all relevant information available for further processing.

The error would only be set when either a network error or a function relay error is encountered. Otherwise, you'd check response.ok and/or response.status properties as you're used to with fetch.

Would love to get your feedback on this approach, would that work for you?

[jaitaiwan]

My feeling on this is, that there be two functions:

1. Which is designed to help simplify the process of implementing functions (e.g. invoke)
2. Which is designed to provide low level access

My preference would be that they are the same function but once you’ve used the body in response.json() for example, you can’t use other body functions (in my understanding).

Overall I’d like to see supabase be more opinionated about edge function structure and how it’s called etc to reduce the mental workload for folks newer to the platform, and then ofc the power of the platform would be that you’re not forced to use that pattern.

My reasoning on this is that there are a few common use cases which edge functions address which are caused by the nature of the supabase JavaScript library. For example, inviteUserByEmail is a server-side only function so it has to be called somewhere on a server. If you’re wanting to make supabase a server less platform so-to-speak then your two options are Postgres functions with http enabled or edge functions.

[vejja]

Hey @vejja, thanks so much for this PR! We've discussed this a bit more internally and are actually contemplating moving the behaviour of invoke closer to the behaviour of fetch. So rather than processing the content type within invoke, we'd return { response, error } where response is actually the response returned from fetch. Then you'd have all relevant information available for further processing.

The error would only be set when either a network error or a function relay error is encountered. Otherwise, you'd check response.ok and/or response.status properties as you're used to with fetch.

Would love to get your feedback on this approach, would that work for you?

Hi @thorwebdev

Funny because I've been turning my head around the same question for the last few days !
I am going to give a long and detailed explanation below as I have thought about it a lot on my side

But in short I would advocate not to go this way and here is why:

1. It would degrade DX rather than improve it, by forcing the developer to write a lot of boilerplate code to deserialize the Response and check the status codes manually
2. It would depart from the behavior of other Supabase APIs (such as the PostgrestClient and the Auth API) which all return { data, error }
3. It would depart from other libraries such as Firebase, which returns {data} and throw on error

I would like to elaborate a bit on my point number 1 above, which is ultimately the most crucial to me.

First, I'd like to testify how superior the client-side DX is with Supabase.

Think about Firebase's horrendous modular library where you need to import collection, getDoc, ref, query, and then stack up these function calls in reverse order just to query the database. Not to mention the infamous ref namespace collision when you try to work with database and storage simultaneously, or the overly complex onSnapshot semantics.

In comparison, the Supabase syntax of from().select().eq() and from().on() is a DX dream of natural simplicity.

In my case, it played a huge part in adopting Supabase quickly, so I would try as hard as I can to stand with this approach in the functions-js API.

Now to the heart of DX with invoke()

I would believe that 80% of the use case for edge functions is sending some JSON body and receiving some JSON body.
(Probably the remaining 20% would be some image processing or file manipulation - my guess).

• On the sending side, I do believe that invoke() is already too complex and should be simplified. There is too much boilerplate involved when calling invoke('function-name', { body: JSON.stringify(data), headers: {'Content-Type':'application/json'}}). It could be simplified down to invoke('function-name', data)
• On the receiving side, if you go the fetch route, we will have to manually check error, then response.ok, then response.status, then await response.json() (and maybe even response.headers before that) just to get our hands on the data that we are looking for. Which is all unnecessary boilerplate in my opinion, and could be simplified down to const {data} = invoke(...)

From a DX perspective, I would imagine that the vast majority of Supabase users are using JAMstack frameworks such as Next or Nuxt. So what is their typical workflow ?
1- fetch some data, display it in a reactive template
2- if error, alert and move on

Given that the major JAMstack frameworks already have some built-in functionalities to catch errors (and optionally report them automatically in the likes of Sentry and Datadog), the real developer's job is to write the code for point number 1.

In light of this, we are then going to end up in a situation where the devs end up re-writing always the same 10 lines of boilerplate each time they use invoke, and end up questioning why we didn't make it simpler for them.

So what kind of solution can be implemented?

In my ideal world, here is how it would work:

• On the sending side, just call invoke('function-name', data). Invoke would then inspect the type of data that is being sent, and depending upon whether it is a Blob, a File, a FormData, or a plain object, set the corresponding headers, serialize accordingly, and send to the endpoint. invoke would have a third 'options' argument where we could manually override the data type and headers.
• On the receiving side, use the result of that call with const {data, error} = await invoke(), which syntax is nicely similar to the from API, the rpc API, the storage API and the auth API. invoke would inspect the type of data that is being received, and depending on it would gently deserialize it automatically before returning it into data. Which is what it already does currently, so no change needed here, except that we could also use another 'options' parameter to also override the type deserialization if needed.
• On error, use either if (error) {} or catch (error) {}, similar also to the from, rpc, storage, and auth APIs. Now error is trickier as this conversation has shown because we have several potential unrelated sources of error and we all know that the JS Error interface is far from ideal in that respect. But on balance I do think we are not that far away from a comprehensive solution, because if you think about it we only have a limited number of potential errors:
  a) Errors arising from fetch
  b) Errors arising from relay
  c) Errors arising from HTTP status code being non-2xx
  d) Errors arising from deserialization (e.g. JSON parse via response.json())
  e) Other errors ?

Getting back to error handling

So what we need to do is simply find a way to indicate whether error is originating from a), b), c), d), or e).
There are 2 ways of doing this, the first one being to add a 'type' property to the error object, and the second one which I think is more Javascript-ish is to subclass Error and use instanceof - this is how this PR was written.

The error-catching part on the client side would then be very intuitive:

if (error instanceof HttpError) {
  // this one has status, statusText, and data properties
  // btw we could rename 'data' to 'details' or 'context' - as 'data' is really horrible
} else if (error instanceof RelayError) {
 // this one doesn't have status and statusText, but has other contextual information
} else if (error instanceof FetchError) {
 // other context provided
} else {
 // etc...
}

I believe in 99% of the cases, the devs will want to react only on the first check, and throw a generic error for the other situations (because what can you do really if it's a relay or fetch issue ?), but at least we give them a nice and easy way to discriminate the different types of errors.

So what does this mean for this PR ?

Several things really, but nothing complex

1. Pull back status and statusText into HttpError rather than returning them in the tuple {data, error, status, statusText}
2. Lose status and statusText from the generic response of invoke() - which I think we don't care because if everything is OK we don't need them
3. Create new subclasses of Error to catch RelayError, FetchError, etc... Wrap all subclasses into a generic FunctionsError interface (optional)

Additional considerations

If we go this route we have two additional considerations to take into account.

The first one relates to losing the richer Response content by returning only {data}.

i.e. vs your suggestion of returning the original response object.

I can see at least two situations were this could be a problem:

• The serverless edge function wants to return a specific 2xx status code, e.g. 201 or 202 and this has a specific meaning that deserves particular treatment on the client side. The issue is that we are losing status.
• The serverless edge function wants to return non-JSON content (such as a stream or a file), but needs to add additional data context via the response headers, such as an id that needs to be extracted client-side. The issue is that we are losing headers.

First, please note that the current implementation of invoke already loses this information, so we are not worse off.

But as I wrote earlier, in my ideal world there would be a third 'options' parameter that overrides the deserialization block. This parameter would be provided as a callback, and that callback would have access to the original Response object, so if the developer really needs to access the original response and use the status or the headers for whatever reason, it will become possible.

The second one relates to what I called simplifying the sending side of invoke

i.e. by writing invoke('function-name', data) and leaving it up to invoke to properly serialize data and add headers.

Similar to my proposal above, there would be an 'options' parameter that overrides the serialization block, and that would also be a callback which would return a request object and feed it up to fetch internally. So here again the developer would be entirely free to override the boilerplate if needed.

Now the crux here is that this would most likely be a breaking change to invoke.
However please note also the following points:

• if you decide to go the fetch route as you were suggesting by returning {response, error} rather than {data, error}, this would also be a breaking change
• in case we have to implement a breaking change, I would strongly prefer the one that reduces boilerplate over the one that increases boilerplate
• With regards to this PR, which only contemplates error handling at this stage, there is no breaking change involved
• Edge functions are still beta, so it is still time to implement breaking changes

Conclusion

If we go my ideal world route, then the invoke prototype would look like this:

const { data: any, error: FunctionsError } = await invoke('function-name': string, reqData: any, options?: {requestTransform, responseTransform})

• with 'requestTransform' taking reqData as argument and returning a Request object. In case no 'requestTransform' is provided, invoke would do the standard serialization of reqData based on its type and add the good headers;
• and 'responseTransform' taking the Response object as argument and returning data. In case no 'responseTransform' is provided, invoke would do the same standard de-serialization of response as it is doing right now.

I am aware that I wrote a very, very, very long comment on that thread, but if you read me up to this line then probably it means it was worth it.

Many many thanks for soliciting my feedback on that one.

[vejja]

My feeling on this is, that there be two functions:

1. Which is designed to help simplify the process of implementing functions (e.g. invoke)
2. Which is designed to provide low level access

My preference would be that they are the same function but once you’ve used the body in response.json() for example, you can’t use other body functions (in my understanding).

Overall I’d like to see supabase be more opinionated about edge function structure and how it’s called etc to reduce the mental workload for folks newer to the platform, and then ofc the power of the platform would be that you’re not forced to use that pattern.

My reasoning on this is that there are a few common use cases which edge functions address which are caused by the nature of the supabase JavaScript library. For example, inviteUserByEmail is a server-side only function so it has to be called somewhere on a server. If you’re wanting to make supabase a server less platform so-to-speak then your two options are Postgres functions with http enabled or edge functions.

Hey @jaitaiwan

I just wrote a very very detailed comment which I think goes exactly your way, i.e.

• reduces boilerplate for newer folks
• is opinionated by default but still provides full-powered low-level access on option
• is only in one function

Your comment is more concise but I agree 100%
Cheers

[jaitaiwan]

@vejja YES! Yours is fantastic. Completely agree with the conclusion and ensuring we have a unified api throughout the codebase.

[vejja]

Hi all,
I can see that there are several positive reactions to my comment above.
I will provide an implementation proposal but under another issue & branch in order to separate the topics in a cleaner way

[inian]

Has been addressed in supabase-js v2.