remove custom Result type from @inlang/core/query

[samuelstroschein]

Problem

The custom Result type is non-standard, not-serializable, and thus leads to more problems than it solves?

• Non-standard is bad because the concept of Result and API usage needs to be explained to developers who use @inlang/core to build apps on inlang.
• Not-serializable means the result can't be returned from a server and used on the client.

Both problems could be solved. But more problems emerged:

• More often than not, functions that return a Result use try catch internally to wrap external APIs. The returned error becomes unknown by default.
• The error type is not generic and therefore not typesafe. Useful matching like if (result.error === "Message not found") is not possible.

Alternatives

• Use throw with JSDoc
• Use union types a la { isOk: boolean, value?: T, error?: E}

Additional remarks

Interesting how other APIs deal with this issue (Firebase, Supabase, etc.)

[samuelstroschein]

Leaning toward the removal of Result. The result behavior is unusual, does not mix nicely with other libraries that expect a throw, and seems to add too little to negative benefit.

Supabase changed to a result-based API supabase/supabase-js#32. Some comments resemble the DX problems of using such API in libraries like SWR. Other users have followed with the request to have an unwrap method as we do now supabase/supabase-js#92. Having methods prevents serialization though. One of the reasons why I want to get rid of result.

[samuelstroschein]

A type-only Result that can be extended with proxies in the future could be a solution.

type Result<T, E> = { data: T, error?: undefined } | { data?: undefined, error: E }

function x(): Result<string, "something wen't wrong"> {
  return { data: "hello" }
}

const { data, error } = x()

console.log(data)

• Very simple.
• Serialization works.
• No function method is returned.
• Provides the value of Result. Namely, developers know when to expect errors.

We could provide additional utilities like unwrap in a separate function to play nicely with external APIs

// will throw if an error has been returned. 
const data = unwrap(x())

[ivanhofer]

Reads exactly like exceptionally. I could add a serialize wrapper that returns a { data, exception } object or tuple and the unwrap functionality that throws an error.

[samuelstroschein]

Yeah but it requires manually serializing things. I ran into this issue before with Result. The moment functions are involved like result(), serialization breaks apart. Developers will end up wondering why the code doesn't work in RPCs and similar server-to-client use cases.

[ivanhofer]

import { success } from 'exceptionally/serialize'

function x() {
  return success('hello')
}

const { data, error } = x()

?

[samuelstroschein]

Okay, but then the rest of exceptionally is not required / we wouldn't want to mix serializable results with non-serializable results. No need for success etc.

FYI the benefit of Result long-term (3+ years) is probably negative once TypeScript implements microsoft/TypeScript#13219. Throwing is JavaScript's Result. But, who knows when TS is implementing this.

Are you okay with using the existing Result type for now?

[ivanhofer]

The success function attaches the whole TypeScript magic to the returned data/exception.

I don't really see the benefit of using it because it still can throw errors if you don't pay attention:

And we probably won't use Result in client code as it is boiler-platy. There we will need another solution.

[samuelstroschein]

@ivanhofer I vote for a simple union type that optimizes for DX of consumption. Two proposals:

Proposal A object syntax

• ⚪ explicit return value e.g. { error: "something" } but more boilerplate
• ❌ consumption is harder due to redeclarations

type ResultA<T, E> = { data: T, error?: undefined } | { data?: undefined, error: E }

function getRefernceMessageA(): ResultA<string, "something went wrong"> {
  return  { error: "something went wrong"}
}

function getMessageA(): ResultA<string, "something went wrong"> {
  return { data: "Willkommen zu dieser App." }
}

// ------------------------------------------------------------------
// consumption is annoying and requires an unwrap. using two results in one
// code block will always lead to re-declarations of the return values. 
const { data, error } = getRefernceMessageA()
const { data, error } = getMessageA()
// -------------------------------------------------------------------

const referenceMessageA = getRefernceMessageA()
const targetMessageA = getMessageA()

if (referenceMessageA.error || targetMessageA.error){
  throw Error("Query went wrong")
}

// both values are defined now
referenceMessageA.data
targetMessageA.data

Proposal B tuple syntax

• ✅ nice consumption DX (no unwrap required)
• ✅ less boilerplate when returning a success e.g. just wrap it in [value]
• ❌ returning an error requires an "odd" comma [, error]

type ResultB<T, E> = [data: T, error?: undefined ] | [ data: undefined, error: E ]


function getRefernceMessageB(): ResultB<string, "something went wrong"> {
  return  [, "something went wrong"]
}

function getMessageB(): ResultB<string, "something went wrong"> {
  return  ["Willkommen zu dieser App."]
}


const [ referenceMessage, errorReference ] = getRefernceMessageB()
const [ targetMessage, errorTarget ] = getMessageB()

if (errorReference || errorTarget){
  throw Error("Query went wrong.")
}

// both types are defined now
referenceMessage
targetMessage

[samuelstroschein]

const [ referenceMessage, errorReference ] = getRefernceMessageB()
const [ targetMessage, errorTarget ] = getMessageB()

This consumption pattern makes Proposal B better in my opinion. The reason I use unwrap everywhere with the current Result type is precisely to avoid re-declarations. With Proposal B, unwrap to avoid re-declarations is not required anymore

[ivanhofer]

https://github.com/ivanhofer/exceptionally#comparisons ^^
I find the tuple syntax better than the object syntax

[samuelstroschein]

https://github.com/ivanhofer/exceptionally#comparisons

I took a look. The additional value of using exceptionally over a one-line type is marginal. On the flip side, the concepts that exceptionally introduce need additional explanation. IMO the trade-off between simplicity (one line Type) and marginal improvement but more complex implementation is not worth it.

I would agree to refactor Result to be a tuple type. If you agree, I will open a PR today that removes the old Result type.

Should I open a PR?

---

FYI the examples in exceptionally seem biased and/or the implementation contains bugs. In any case this is exactly why strongly vote for a simple type. No (less) explanation or debate about a correct implementation needed 😃

The exceptionally example states that "we are also save if a function returns 0". I assume that exceptionally implicitly returns an exception for falsy values. If that is the case, I consider this to be a bug. By no means does a (user) count of 0 mean an exception occurred. (The pattern of returning 0 for failing functions as in C does not exist in JavaScript)

// from the exceptionally example

// example 2
const userCountResult = db.countUsers()
if (userCountResult.isException) throw Error('oops') // we are also save if the function returns `0` 

The tuple example does need to check if userCount is undefined. If the tuple returns an error variable, the following code:

// from the tuple example

// example 2
const [userCount] = db.countUsers()
if (!userCount) throw Error('oops') // but what if the function returns 0?
// we would always need to check it like this
if (userCount === undefined) throw Error('oops')

Becomes this two line code:

// from the tuple example

// example 2
const [userCount, userCountError] = db.countUsers()
if (userCountError) throw Error('oops') 

// userCount is not undefined after checking if an error occured 

Basically, identical to the exceptionally example. If you refer to checking for undefined because TypeScript's type system is not sound, i.e. someone declares a type can't be undefined but is undefined on runtime, then this is a limitation of TypeScript that applies universally.

[samuelstroschein]

RE checking for undefined:

Aha, I watched your talk https://www.youtube.com/watch?v=oteOKQIYiqo. You have to manually check for undefined if the return type of a function is not explicitly set to Result<Data, Error>.

[ivanhofer]

Sure those are made up examples.. But I saw all of them in production!
Developers also need to learn that

const [userCount, userCountError] = db.countUsers()
if (!userCount) throw Error('oops') // wrong
if (userCount === undefined) throw Error('oops') // also wrong
if (userCountError) throw Error('oops') // correct

those three checks are all different.

I assume that exceptionally implicitly returns an exception for falsy values

No it returns what you use to wrap the return value.
You can have an exception of true and a success of undefined if you want.
The return value does not decide whether it is an exception or a success. This make sit also possible to return in a void case. If you don't check it like this if (userCountError) throw Error('oops') then you have introduced a bug in your code. That's what I meant with "you are not enforced to handle exceptions". If you use the tools wrong, then it is your fault. But I find it to easy to mess it up. That's why exceptionally exists.

I don't have problems using the tuple approach, because I know how to use it correclty. But this does not mean I won't ever use it incorrectly after itting 10 hours in front of the pc ^^.

It is certainly better than the Result approach, because it does not throw unless you really want it. But still we need to make sure that everyone uses it correctly.

[samuelstroschein]

Implemented tuple based result.