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

Closed

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
Contributor

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.

Copy link
Contributor

@braxtonhall braxtonhall left a comment

Choose a reason for hiding this comment

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

Nice! I think it might be even nicer though if the original values are not lost when calling .andFinally

As a user, I think I expect that finally used for side effects, and probably shouldn't change the type of the result.

For example with promises (linked in your PR description):

(async () => {
	const resolveFinally = await Promise.resolve("foo")
		.then((a) => a)
		.finally(() => "bar");

	const rejectFinally = await Promise.reject("foo")
		.catch((a) => a)
		.finally(() => "bar");

	console.log(resolveFinally, rejectFinally); // prints '"foo" "foo"'
})();

I guess the try/finally in JavaScript is a counter-example, because the finally block can overwrite the return in the try or catch block, but in the case of no return statement in the finally block, the original return value in the try or catch block is used.

It would be nice to be able to still observe that an error occurred even after the andFinally callback executes. Here's an example that looks just like JavaScript promises:

const doStuff: Result<string, number> = ok('start db connection')
	.andThen(() => {
		// do stuff that might return an err()
		const result: Result<string, string> = ok('do something')
		return result
	})
	.andFinally(() => {
		// close the db connection
	})
	.orElse(() => {
		// trigger some alarm!
		return err(1);
	})

... although this just looks suspiciously like #456

@kishore03109
Copy link
Author

Closing because some of the comments made by @macksal & @braxtonhall were valid, and was not construct a neat way to address their concerns

@macksal
Copy link
Contributor

macksal commented Oct 1, 2024

Hi @kishore03109 I've added a new implementation in #588 with a slightly different design, please consider reviewing if you find it useful!

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.

3 participants