vultix / ts-results

A typescript implementation of Rust's Result object.
MIT License
1.16k stars 69 forks source link

Err.unwrap()'s behavior and the object it throws #48

Open jstasiak opened 2 years ago

jstasiak commented 2 years ago

Err.unwrap() does this right now

    unwrap(): never {
        throw new Error(`Tried to unwrap Error: ${toString(this.val)}\n${this._stack}`);
    }

which kind of makes things inconvenient for my use case. This is the plan I had to gradually introduce ts-results into a project – convert

class AuthError extends Error {}

function authMe(parameter: string): string {
    if (parameter !== 'good') {
        throw AuthError()
    }
    return 'your token'
}

fuction someLongFunction() {
    // ...
    const token = authMe(some.piece.of.data.from.somewhere);
}

to

class AuthError extends Error {}

function authMe(parameter: string): Result<string, AuthError> {
    if (parameter !== 'good') {
        return Err(AuthError())
    }
    return Ok('your token')
}

fuction someLongFunction() {
    // ...
    const token = authMe(some.piece.of.data.from.somewhere).unwrap();
}

The point is that the project's error handling is exception-based right now and different actions (HTTP responses for example with different HTTP status codes) are taken based on the exception types and properties.

I assumed unwrap() was actually throwing the original error which would make it possible to perform a greadual conversion like above while keeping the exact same behavior. I was wrong and I'm wondering what can be done about it. It's not even possible to modify our code to handle the errors thrown by unwrap() because the errors thrown don't have a reference to the original errors.

I see the following options:

  1. Modify unwrap() to throw the original value instead of creating a new one. Upsides: no new API introduced Downsides: we no longer get the Tried to unwrap Error: ... message, it would be backward incompatible in a way
  2. Modify the error thrown to have a reference to the original error like so

    unwrap(): never {
        throw new Error(`Tried to unwrap Error: ${toString(this.val)}\n${this._stack}`, this.val);
    }

    Upsides: no new API introduced Downsides: handling of the errors is slightly more verbose

  3. Add a new API to throw just the original error value Upsides: unwrap() remains focused on what it does, concerns are separated Downsides: it's a new API and increases complexity, requires a name (can't say I know what I'd call it)
m-shaka commented 2 years ago

Hi. I think option 1 is best even though it's a breaking change. Can I hear any updates?

ozanmuyes commented 1 year ago

I vote for the Option 1. In order to maintain backward compatibility I have some ideas;

  1. Check to see if this.val is an instance of Error (at https://github.com/vultix/ts-results/blob/master/src/result.ts#L150), only if so throw that error instead of wrapping it.
  2. In addition to 1, where we check whether the value is an instance of Error, for such developers that has edge-case like myself, check to see if this.val is of type object and owns a special, unique key - e.g. _isError (and of course, if it's a truthy value). In a project that I've been working on (clean architecture-ish) I have this CoreError which is the base class for other, specific errors that it doesn't extend the Error object. So this special key on the object that is Err variant of a Result might be an escape hatch for me and alike.
  3. In the case of this.val neither an instance of Error or doesn't own this special, key wrap the value with an Error as in the current implementation. This may be counted as an edge-case but users are not forced to define Err variant of a Result as an Error or even an object.
jstasiak commented 1 year ago

FYI after I read about JS error cause in https://github.com/vultix/ts-results/issues/34#issuecomment-1081709850 I decided to go with option 2 in the ts-results fork that we use in our code, https://github.com/lune-climate/ts-results-es/commit/73a700ee489458a0d2a157b88d1256eb4940e01e

rafagalan commented 1 year ago

This (either option 1 or 2) brings enormous value to me. Can I help with this to speed up the release?

jstasiak commented 1 year ago

You can try our fork where option 2 is implemented: ts-results-es

stephen776 commented 1 year ago

You can try our fork where option 2 is implemented: ts-results-es

Hi - I am currently attempting this using your fork but I can’t seem to find the relevant info in the docs.

Would you be able to point to or provide an example of accessing the original err value?

jstasiak commented 1 year ago

Oh yeah, that's a good point. https://github.com/lune-climate/ts-results-es/pull/50 should address that (to an extent) – you access it through the cause property[1].

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause

pedrro commented 10 months ago

Hi team, I'm facing exactly the same issue, does we have any eta to update the main project with the fix that are in es? @jstasiak

tncbbthositg commented 10 months ago

By the way, the documentation seems to imply that the desired behavior is the actual behavior: https://github.com/vultix/ts-results#unwrap

let goodResult = new Ok(1);
let badResult = new Err(new Error('something went wrong'));

goodResult.unwrap(); // 1
badResult.unwrap(); // throws Error("something went wrong")

@vultix would you entertain a PR that throws an UnwrapError : Error that includes a cause: Error property? I think that would be unlikely to cause many breaking changes.