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

feat(andFinally): add andFinallyFunctionality #536

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kishore03109
Copy link

Description

Add 'finally' functionality to neverthrow. Overall leads to clearer typing with less verbose code. The mental model is similar to the native finally.

This PR attempts to solve 2 main issues. Consider the example below:

    const doesResourceExist = async (): Promise<true> => {
      const simulateError = () => Math.random() < 0.33
      if (simulateError()) {
        throw new Error('Unrecoverable error')
      }
      if (simulateError()) {
        throw new Error('404 Not found error')
      }
      return true
    }

    const safeFunction: ResultAsync<true, ResultAsync<boolean, unknown>> = ResultAsync.fromPromise(
      doesResourceExist(),
      (error) => error,
    ).mapErr((error) => {
      if (`${error}` === '404 Not found error') {
        return okAsync(false)
      }
      return errAsync(error)
    })

Notice the return type of the safeFunction is typed to an unintuitive type. Not sure if there was a convenient way for error recovery while mutating the Ok type. With andFinally, there is easier control over the Ok type during error recovery as such:

    const improvedSafeFunction: ResultAsync<boolean, unknown> = ResultAsync.fromPromise(
      doesResourceExist(),
      (error) => error,
    ).andFinally((error) => {
      if (`${error}` === 'Not found error') {
        return okAsync(false)
      }
      return errAsync(error)
    })

Additionally, this solves #525. As documented in the issue, the below code becomes less verbose.

Before

    const doStuff: Result<string, never> = ok('start db connection')
      .andThen(() => {
        // do stuff that might return an err()
        const result: Result<string, string> = ok('do something')
        return result
      })
      .andThen(() => {
        return ok('close db connection')
      })
      .orElse(() => {
        return ok('close db connection')
      })

After

    const doStuff: Result<string, never> = ok("start db connection").andThen(() => {
      // do stuff that might return an err()
      const result: Result<string, string> = ok("do something")
      return result
    }).andFinally(() => {
      return ok("close db connection")
    })

Testing

Have added some minimal tests to capture correct behaviour.

return new ResultAsync(
this._promise.then((res) => {
if (res.isErr()) {
return f(res.error)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have a few questions here:

  • Doesn't this imply that f should be typed as f: (t: T | E) => R? (since you're calling it with res.error)
  • Is there a reason we need newValue instanceof ResultAsync ? newValue._promise : newValue in the ok branch but not in the isErr() branch?
  • Why even unbox the type instead of just passing the Result object to f? The union type seems unergonomic and erases whether an error occurred or not.

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

Successfully merging this pull request may close these issues.

None yet

2 participants