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

.match with functions that return Result<., .> #594

Open
timvandam opened this issue Oct 12, 2024 · 12 comments
Open

.match with functions that return Result<., .> #594

timvandam opened this issue Oct 12, 2024 · 12 comments

Comments

@timvandam
Copy link

timvandam commented Oct 12, 2024

Currently it does not seem possible to handle both Ok and Err at the same time. Note that this behavior is different from .andThen(...).orElse(...) or .orElse(...).andThen(...) as the first of the two chained functions may have its return value handled by the second of the two. As far as I am aware there is no neat way of handling this currently. .match comes close, but returns a Promise rather than ResultAsync when applied to ResultAsync.

I propose adding the following:

declare class ResultAsync<T, E> implements PromiseLike<Result<T, E>> {
  fork<A, B>(ok: (t: T) => Result<A, B> | ResultAsync<A, B>, _err: (e: E) => Result<A, B> | ResultAsync<A, B>): ResultAsync<A, B>
}

I am willing to submit a PR if this idea is accepted

@timvandam
Copy link
Author

timvandam commented Oct 12, 2024

The pattern I have adopted in the meantime is:

fromSafePromise(x.match(asyncFn1, asyncFn2).then(id)).andThen(id)

Where id is an identity function that serves to 1) flatten the promise returned by the match branch handler (required because match does not expect async functions. a asyncMatch may be handy for this case), and 2) to flatten the Result (required because I return Result inside of the match branch handlers)

@paduc
Copy link
Contributor

paduc commented Oct 12, 2024

Hi @timvandam,

I’m having a hard time understanding what you want to do. Could you provide a more complete example ?

@timvandam
Copy link
Author

Hi @timvandam,

I’m having a hard time understanding what you want to do. Could you provide a more complete example ?

What I’m trying to achieve is behavior analogous to Promise.then with 2 arguments: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then#onrejected. This is not the same as chaining .then and .catch (and similarly .andThen and .orElse can nit be applied to achieve the same behavior in a simple way)

You provide two functions that handle the ok/err cases and return a (Async)Result

@paduc
Copy link
Contributor

paduc commented Oct 14, 2024

Thanks @timvandam. Reading this also helped me grasp the topic.

Wouldn't the best solution be to mimick Promise.then and accept a second function argument to Result.andThen, instead of adding a new fork ?

@timvandam
Copy link
Author

@paduc Yes that would be fine too. I personally would prefer fork because its much clearer in my opinion. And neverthrow does not necessarily follow the Promise naming (e.g. by using orElse rather than something like catch) so I think it would be nicer

@macksal
Copy link
Contributor

macksal commented Oct 18, 2024

I like this because it kind of completes the "toolbox".

For mapping to a result:

  • andThen (only runs for Ok)
  • orElse (only runs for Err)
  • fork (provide different paths for Err and Ok)

For mapping to a value/err:

  • map (runs for Ok)
  • mapErr (runs for Err)
  • match (provide different paths for Err and Ok, and also unboxes the value/err)

On the other hand, can you already accomplish the same thing by just returning a ResultAsync from both branches of your match?

@timvandam
Copy link
Author

timvandam commented Oct 18, 2024

@macksal achieving the behavior of fork by using andThen and orElse is possible, but not a nice pattern, e.g.:

resultAsync
  .andThen(async (val) => {
    if (await condition(val)) {
      return ok(val);
    }

    return err({ passthrough: true, error: new Error('condition not met') });
  })
  .orElse(async (val) => {
    if ('passthrough' in val && val.passthrough) return err(val.err);

    // normal error handling logic here
  })

The difference between fork and a combination of andThen and orElse is also clear by considering the differences for Promise: in .then(.).catch(.), the catch function can capture exceptions thrown in the then function. Similarly, in .catch(.).then(.), handling an error in the catch function will cause the then function to run next. This is clearly different from .then(a, b) because a and b will never both be called. The same difference would apply for fork, andThen, and orElse.

Edit:

I misread your question - you are talking about match. This is not possible as match returns a Promise type rather than a ResultAsync. However you probably could return a Result, wrap the match in fromSafePromise and then flatten the ResultAsync<ResultAsync, never> with .andThen(x => x)

match<A, B = A>(ok: (t: T) => A, _err: (e: E) => B): Promise<A | B> {
return this._promise.then((res) => res.match(ok, _err))
}

@macksal
Copy link
Contributor

macksal commented Oct 18, 2024

@timvandam understood. It works normally for sync but not for async. fork seems justifiable to me.

@paduc
Copy link
Contributor

paduc commented Oct 18, 2024

In my opinion, it's valuable to have an API with a clear distinction between the Result/ResultAsync "world" (eg andThen, orElse, map, mapErr, ... all methods with a Result => Result signature) and the Promise world, for which neverthrow offers entries (eg fromPromise and other Promise => Result) and exits (eg unwrap, match and other Result => Promise).

When there is a back and forth between these worlds, it starts to smell like bad code.

I'm all for adding methods like fork that helps us keep our code in the Result/ResultAsync side.

@dirkluijk
Copy link

dirkluijk commented Nov 27, 2024

Just stumbled upon this conversation, when I wanted to create a new issue about the same topic.

I just started playing around with neverthrow and I really like its design, but it would be great if there are some overloads that allow plain values that will be "normalized" down under the hood.

A good example is the orElse() method/operator that only accepts another Result (or AsyncResult). In promises, if this callback throws an error, it is properly caught. It would be great if neverthrow does the same.

I already fiddled around with it, and I think it should be feasible, without adding breaking changes and without loosing type-safety.

Example:

export class ResultAsync<T, E> implements PromiseLike<Result<T, E>> {
  orElse<R extends Result<unknown, unknown>>(
    f: (e: E) => R,
  ): ResultAsync<InferOkTypes<R> | T, InferErrTypes<R>>
  orElse<R extends ResultAsync<unknown, unknown>>(
    f: (e: E) => R,
  ): ResultAsync<InferAsyncOkTypes<R> | T, InferAsyncErrTypes<R>>
  orElse<U, A>(f: (e: E) => Result<U, A> | ResultAsync<U, A>): ResultAsync<U | T, A>
  orElse<R>(f: (e: E) => R): ResultAsync<R | T, E> // added this one
  orElse<R>(
    f: (e: E) => Result<R, unknown> | ResultAsync<R, unknown> | R,
  ): ResultAsync<unknown, unknown> {
    // ...
  }
}

Example of a type-guard that can be used under the hood:

export function isResult<T, E>(result: Result<T, E> | unknown): result is Result<T, E> {
  return result instanceof Ok || result instanceof Err
}

Example of a normalize function that can be used under the hood:

export function normalize<T, E>(value: ResultAsync<T, E> | Result<T, E> | T): ResultAsync<T, E> {
  return value instanceof ResultAsync
    ? value
    : new ResultAsync<T, E>(
        Promise.resolve<Result<T, E> | T>(value).then((it) => (isResult(it) ? it : ok(it))),
      )
}

Is this an idea that could be considered? I'd be happy to submit a PR for this.

@paduc
Copy link
Contributor

paduc commented Nov 27, 2024

This is how I normalize:

const myResultAsync = okAsync().andThen(() => myUnknownResult);

@dirkluijk
Copy link

dirkluijk commented Nov 27, 2024

Well, my problem is not really that it's not possible with the current API, but it's just a bit cumbersome. I am trying to combine some generic utils (that are not coupled to neverthrow) like this:

myResultAsync
    .orElse(ifInstanceOf(ProblemError, convertProblemError))
    .orElse(ifInstanceOf(HttpStatusError, convertHttpStatusError))
    .orElse(ifInstanceOf(MyCustomError, convertMyCustomError))
    .unwrapOr(response({ message: 'Internal server error' }, { statusCode: 500 }));

instead, I need to write:

myResultAsync
    .orElse(fromThrowable(ifInstanceOf(ProblemError, convertProblemError)))
    .orElse(fromThrowable(ifInstanceOf(HttpStatusError, convertHttpStatusError)))
    .orElse(fromThrowable(ifInstanceOf(MyCustomError, convertMyCustomError)))
    .unwrapOr(response({ message: 'Internal server error' }, { statusCode: 500 }));

(for context, my ifInstanceOf looks like this, but I don't want it to be tied to neverthrow to allow users to use this helper also in plain promises)

function ifInstanceOf<TError, TResult>(errorClass: Class<TError>, errorConverter: (error: TError) => TResult) {
    return (e: unknown) => {
        if (e instanceof errorClass) {
            return errorConverter(e);
        }
        throw e;
    };
}

Which seems a bit of a missed opportunity.

Of course I can just use a plain Promise for this:

return myPromise
    .catch(ifInstanceOf(ProblemError, convertProblemError))
    .catch(ifInstanceOf(HttpStatusError, convertHttpStatusError))
    .catch(ifInstanceOf(MyCustomError, convertMyCustomError))
    .catch(() => response({ message: 'Internal server error' }, { statusCode: 500 }));

but I would really like better interoperability with neverthrow, since this API has way better semantics.

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

4 participants