feat: Add andFinally functionality and tests

macksal commented 1 month ago

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(connection => 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: ```ts 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[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 69bb69745627a34684927bec13f19cbd702f56b2

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 commented 1 month ago

@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).

supermacro / neverthrow