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

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

Open
jstasiak opened this issue Mar 11, 2022 · 9 comments
Open

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

jstasiak opened this issue Mar 11, 2022 · 9 comments

Comments

@jstasiak
Copy link
Contributor

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)

@jstasiak jstasiak changed the title Err.unwrap() behavior and the object it throws Err.unwrap()'s behavior and the object it throws Mar 11, 2022
jstasiak added a commit to lune-climate/ts-results-es that referenced this issue Mar 29, 2022
We need TypeScript 4.6+ to be able to construct Error objects with the
cause property[1]. The property is useful to chain errors and, on the
catching side, to see what error caused the error we just caught.

I want to have this in place to implement [3] which satisfies a need we
ourselves have[4].

tslib upgraded in lockstep because otherwise we get

    FAIL  test/result.test.ts
      ● Test suite failed to run

        test/result.test.ts:121:29 - error TS2807: This syntax requires an imported helper named '__spreadArray' with 3 parameters, which is not compatible with the one in 'tslib'. Consider upgrading your version of 'tslib'.

        121     const all4 = Result.all(...([] as Result<string, number>[]));
                                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error
[2] microsoft/TypeScript#45167
[3] vultix#34
[4] vultix#48
jstasiak added a commit to lune-climate/ts-results-es that referenced this issue Mar 29, 2022
I want to have this in place to implement [1] which satisfies a need we
ourselves have[2]. This is needed because the cause property is only
availble in the es2022 TS profile[3].

[1] vultix#34
[2] vultix#48
[3] microsoft/TypeScript#47020
jstasiak added a commit to lune-climate/ts-results-es that referenced this issue Mar 29, 2022
This is useful to get access to the "underlying" error in global
exception-handling code for example.

Feature requested in [1] and I had a question about something similar as
well[2].

[1] vultix#34
[2] vultix#48
jstasiak added a commit to lune-climate/ts-results-es that referenced this issue Mar 29, 2022
We need TypeScript 4.6+ to be able to construct Error objects with the
cause property[1]. The property is useful to chain errors and, on the
catching side, to see what error caused the error we just caught.

I want to have this in place to implement [3] which satisfies a need we
ourselves have[4].

tslib upgraded in lockstep because otherwise we get

    FAIL  test/result.test.ts
      ● Test suite failed to run

        test/result.test.ts:121:29 - error TS2807: This syntax requires an imported helper named '__spreadArray' with 3 parameters, which is not compatible with the one in 'tslib'. Consider upgrading your version of 'tslib'.

        121     const all4 = Result.all(...([] as Result<string, number>[]));
                                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error
[2] microsoft/TypeScript#45167
[3] vultix#34
[4] vultix#48
jstasiak added a commit to lune-climate/ts-results-es that referenced this issue Mar 29, 2022
I want to have this in place to implement [1] which satisfies a need we
ourselves have[2]. This is needed because the cause property is only
availble in the es2022 TS profile[3].

[1] vultix#34
[2] vultix#48
[3] microsoft/TypeScript#47020
jstasiak added a commit to lune-climate/ts-results-es that referenced this issue Mar 29, 2022
This is useful to get access to the "underlying" error in global
exception-handling code for example.

Feature requested in [1] and I had a question about something similar as
well[2].

[1] vultix#34
[2] vultix#48

This change depends on #17 and #19 to be merged first (then the branch will be updated to contain these changes and the build will succeed).
@m-shaka
Copy link

m-shaka commented Jul 23, 2022

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

@ozanmuyes
Copy link

ozanmuyes commented Jan 9, 2023

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
Copy link
Contributor Author

FYI after I read about JS error cause in #34 (comment) I decided to go with option 2 in the ts-results fork that we use in our code, lune-climate@73a700e

@rafagalan
Copy link

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

@jstasiak
Copy link
Contributor Author

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

@stephen776
Copy link

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
Copy link
Contributor Author

jstasiak commented Aug 3, 2023

Oh yeah, that's a good point. lune-climate#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
Copy link

pedrro commented Nov 15, 2023

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
Copy link

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.

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

No branches or pull requests

7 participants