feat: Add andFinally functionality and tests

[macksal]

This PR is related to the earlier #536 which proposed an implementation for an andFinally method that will be called regardless of whether an earlier step contained an error or not. This would allow the caller to avoid awkward constructs which pass the same or similar callback to both map/mapErr or andThen/orElse.

The earlier implementation had some problems, and I believe the design still needed some considerations. Primarily, the earlier version did not allow for asynchronous cleanup logic which is a common use case for resources such as database connections, login sessions, etc.

The primary use case considered is one similar to the following. More broadly, the feature is designed to cover use cases that would be normally covered by the finally block in either synchronous or asynchronous code.

function createConnection(): ResultAsync<Connection, NetworkError | AuthenticationError> {
}
function doStuffWithConnection(connection: Connection): ResultAsync<void, NetworkError | BusinessLogicError> {
}
function terminateConnection(connection: Connection): ResultAsync<void, NetworkError> {
}

function main() {
    return createConnection().andThen(connection =>
        doStuffWithConnection(connection)
            // this will be called regardless of whether doStuffWithConnection errors or not
            .andFinally(() => terminateConnection(connection))
    );
}

Design decisions

This feature was designed explicitly to handle scenarios which would normally be covered by the finally block in code that synchronously throws errors or uses async/await. This allows similar thought patterns easily apply to this library, and such code to more easily be updated to use neverthrow.

Key decisions are outlined below with further context.

1. The andFinally callback does not receive a parameter.

In the finally block, I'm not aware of any languages that allow the programmer to directly check if an error has occurred or not. So, I elected not to pass any information about the earlier result to the callback. If it's absolutely necessary, the user can always use mutable state declared in an above scope just like they can with finally. (I rarely see this in practice).

Additionally, if the user wants to add separate logic for ok/error cases, I argue that we already have many features to do so andThen, orElse, etc.

I consider this decision to be low-risk since we can always add support for a result parameter to the callback without being a breaking change.

2. ResultAsync.andFinally must return a Result/ResultAsync and `ok` values are ignored.

In a normal finally block, it is possible for errors to be thrown. This is especially true in cases where a resource such as a database connection/transaction is used and the cleanup logic is asynchronous.

The most straightforward way to handle this is to require andFinally to return a Result. It also allows re-use of other functions that return Result/ResultAsync during cleanup. Other approaches require some kind of "sentinel" value to indicate that no error occurred.

OK values are ignored. This does diverge from languages which allow a finally block to contain a return statement, which will override any return value from elsewhere in the function. However, I do not recall seeing this used in practice.

3. When andFinally returns an error, that error will propagate (and overwrite a previous error).

I believe this is the most intuitive way to handle errors from andFinally. Firstly, it maps exactly to how throw is handled inside finally.

If the user wants to ignore errors inside the callback, they are still free to explicitly ignore them, for example:

createConnection().andThen(connection =>
    connection.doStuff().andFinally(() =>
        connection.terminate().orElse((e) => {
            console.warn("Error terminating connection", e);
            return ok(undefined);
        })
    )
)

4. Result.andFinally can return only a synchronous Result and not a ResultAsync.

This decision was made partly to simplify the implementation and avoid the need for any overloads which can make it more difficult to ensure type-safe code.

In practice I would also expect that if a particular piece of cleanup logic is asynchronous, it is likely that the earlier steps' logic is also asynchronous. This would be the case for most use cases I can think of (database connections and other resource-based cleanup).

Low-risk since we can always add an overload that broadens the accepted types without breaking existing code.

If we feel it's important to continue adding support for ResultAsync in Result's methods, I propose that a better path forwards is to add a .asAsync() method to both Ok and Err that simply lifts the result into a ResultAsync. It's also trivial for the user to do the wrapping themselves.

5. ResultAsync.andFinally will call the cleanup function even if the internal promise rejects

Normally, we can make the assumption that ResultAsync's internal promise should only ever reject if someone is misusing the library (e.g. calling fromSafePromise or new ResultAsync with something that's not safe). I decided that the cleanup function should still be called in this case, but we still propagate the rejection.

My justification:

1. If an erroneous rejection occurs inside ResultAsync, it is indicative of a bug and if we don't run the cleanup function we are more likely to leave the whole application in a globally-broken state from the callers' perspective. (Resource leak, hanging database transaction, etc.). I believe the caller is more likely to want the cleanup to run.
2. We can well-define the contract of andFinally to include this feature and the caller will know it will happen.
3. If there are multiple andFinally blocks in the same chain, they will still all be called which is logically consistent.
4. The rejection is not silently absorbed but will instead propagate through the ResultAsync returned from andFinally, and the bug causing it to happen can still be discovered.

It is a little awkward that we can't do the same for synchronous Results since if an error is thrown during an earlier step, the result whose andFinally method we are calling will never exist. A problem I can see is that the contract for Result/ResultAsync differs in this one aspect.

[changeset-bot]

🦋 Changeset detected

Latest commit: 298571a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
neverthrow	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[macksal]

See the PR description for more details about why this was added. Open to hearing concerns about this.

[hosuaby]

Hello @macksal

Nice PR. I am facing the same issue so I came here. It would be great if maintainers gave some attention to this PR.

The only problem I see, is that the finalization function f can return an error ResultAsync object, and in that case, this error will be simply swallowed. And this goes against the philosophy of explicitness that neverthrow follows.

Unfortunately, there is no way around but to create some sort of combined error that will return both original and finalization errors.

Here is how I implemented it in my code:

import { ResultAsync } from 'neverthrow';
import { Throwable } from './throwable';

export class CombinedError<PE, FE> extends Throwable {
  public readonly _tag = 'CombinedError';

  constructor(
    public readonly programError: PE,
    public readonly finalizationError: FE,
  ) {
    super('Both program and finalization have failed');
  }
}

export function andFinally<R, PE, FE>(
  program: ResultAsync<R, PE>,
  finalization: () => ResultAsync<void, FE>,
): ResultAsync<R, PE | FE | CombinedError<PE, FE>> {
  return ResultAsync.fromSafePromise(
    new Promise((resolve, reject) => {
      program.match(
        async (result) => {
          await finalization().match(
            () => resolve(result),
            (finalizationError: FE) => reject(finalizationError),
          );
        },
        async (error) => {
          await finalization().match(
            () => reject(error),
            (finalizationError: FE) => reject(new CombinedError(error, finalizationError)),
          );
        },
      );
    }),
  );
}

[macksal]

@hosuaby I did this to mirror the precedent of throwing inside finally blocks - most languages do not special case this and simply "overwrite" the previously thrown error if there was one. By extension if our andFinally cleanup fails, we will return that error and ignore the error from the existing result (if there is one).

In my eyes the finally use case is a tool for when you want to decouple some cleanup behaviour from the result of everything before it ("just make sure we run this regardless of what happens beforehand")

Therefore if you find yourself wanting to reintroduce that coupling by inspecting the combined error types, those behaviours are probably more related than you thought. You can easily solve this by making your steps+cleanup in the happy/sad paths more explicit. Whatever we do in the library here would be right for some consumers and wrong for others IMO, so let's go with what is most simple

[macksal]

Allowing for either sync or async Results to be returned by the callback. I couldn't foresee any problems with this and it makes the method more flexible.

[macksal]

@mattpocock @supermacro @m-shaka

This functionality has been discussed at a few points in the past, I really appreciate any comments on this implementation. Or if it simply isn't a priority for the library, please let me know! Eager to hear any feedback.

I'm ready to use this in my own applications, but I'd prefer to settle on a particular interface before doing that (instead of e.g. forking off).

[macksal]

@supermacro @m-shaka I'm still interested in adding this functionality to the library, please let me know any feedback

[macksal]

Updated to current master

[Chase-C]

For what it's worth, I'd like to see this merged in so I can easily run some resource cleanup logic in either an Ok or Err branch.

[macksal]

For what it's worth, I'd like to see this merged in so I can easily run some resource cleanup logic in either an Ok or Err branch.

@Chase-C I've failed to get the attention of any maintainers, and it seems there's nothing being added or discussed for close to a year. I can't quite tell if they are simply busy or don't plan to accept new features.

In either case I would definitely contribute to a fork of the repo if one is made, but I probably wouldn't maintain one myself...

[hosuaby]

For what it's worth, I'd like to see this merged in so I can easily run some resource cleanup logic in either an Ok or Err branch.

@Chase-C I've failed to get the attention of any maintainers, and it seems there's nothing being added or discussed for close to a year. I can't quite tell if they are simply busy or don't plan to accept new features.

In either case I would definitely contribute to a fork of the repo if one is made, but I probably wouldn't maintain one myself...

I have actually created one, while facing the limits of neverthrow. So I have created Defectless, which is a fork and heavy rework of neverthrow.

Currently it is a module of my project https://github.com/sapphire-cms/sapphire-cms/tree/master/packages/defectless, I am battle testing it during one iterration, and very soon I will release it as a proper project with its own repository and documentation.