How do you map an error?

[derkbell]

First of all I have to say this is a great library, well documented, explaining the rationale as well. I just can't figure out the equivalent of mapError as it exists in rust. I often find the need to map errors like so:

return Result.fromAsyncCatching(fetch(url, options))
    .mapError((error) => new MyApplicationError('Fetching failed'))
    .map((response) => response.json())
    .mapError((err) => new MyApplicationError('JSON parsing failed'))

Is there an equivalent?

[derkbell]

I'll prepare a PR. Edit: finished the sync part, will continue on async part later today

[everweij]

Hey @derkbell, thanks for submitting this issue!
I was considering adding this functionality myself lately, however, I'm still trying to figure out all the various use-cases that a mapError potentially captures, and whether each use-case the resulting type still makes sense. Let me share some of my thoughts.

At the moment we have Result.try that lets you wrap code that might throw exceptions (also supports async). So in the case of making a fetch call, this would roughly translate to:

class FetchError extends Error {
	readonly type = "fetch-error";
}

class ParseError extends Error {
	readonly type = "parse-error";
}

type Data = { foo: string };

Result.try(
  () => fetch(url, options),
  () => new FetchError("fetch failed"),
).map((response) =>
  Result.try(
    () => response.json() as Promise<Data>,
    () => new ParseError("json parsing failed"),
  ),
); // Result<Data, ParseError> | FetchError>

While the outcome may be the desired result, I don't think this is a very ergonomic and user-friendly solution, so in that sense, I agree we probably need something here, like mapError.

Type-wise I think mapError would have a lot in common with recover: both take the accumulated errors and reduce them to a single value. In the case of recover this is a new value or result-type, and in the case of mapError this will be a new Error.
The use-cases where this makes the most sense I think are either if you want to group or wrap errors that might have happened in earlier operations, or as an alternative for Result.try. For example:

Result.fromAsyncCatching(fetch(url, options))
  .mapCatching(res => res.json())
  .mapError(err => new FetchError("unable to fetch", { cause: err }));

However, if I look at your example, mapError produces a logically correct result, although the resulting type would probably tell the wrong story:

return Result.fromAsyncCatching(fetch(url, options))  // AsyncResult<Response, Error>
    .mapError((error) => new FetchError('Fetching failed')) // AsyncResult<Response, FetchError>
    .mapCatching((response) => response.json() as Promise<Data>) // AsyncResult<Data, FetchError | Error>
    .mapError((err) => new ParseError('JSON parsing failed')) // AsyncResult<Response, ParseError>

In this case, if something went wrong with the request, it will be wrongfully mapped to a ParseError. In order to circumvent this we could do a conditional check inside mapError like so:

return Result.fromAsyncCatching(fetch(url, options))  // AsyncResult<Response, Error>
    .mapError((error) => new FetchError('Fetching failed')) // AsyncResult<Response, FetchError>
    .mapCatching((response) => response.json() as Promise<Data>) // AsyncResult<Data, FetchError | Error>
    .mapError((err) => {
      if (err instanceof FetchError) {
        return err;
      }
      
      return new ParseError('JSON parsing failed')
    }) // AsyncResult<Response, FetchError | ParseError>

This is technically correct, however, personally, I don't like the 'hidden coupling'.

What it seems we're trying to do here, is collocating the mapping of the error with the associated operation. This is similar to what Result.try enables us to do (but is not 'chain'-friendly).

A solution to this use-case might be for mapCatching to take an additional parameter transformError:

Result.try(
	() => fetch(url, options),
	(err) => new FetchError("fetch failed"),
).mapCatching(
	(response) => response.json() as Promise<Data>,
	(err) => new ParseError("json parsing failed"),
); // Result<Data, ParseError> | FetchError>

This would make the mapCatching types a lot more complex, so I probably need some time to think about this.

All in all, I think mapError could be a nice addition, as long as we make it clear in the docs what the use cases and pitfalls are. I also think we might need to extend the mapCatching a bit.

Edit: replaced map with mapCatching in the examples.

[everweij]

Also, not sure if we need a mapErrorCatching variant. Technically exceptions could be thrown when mapping the error, but I sense this is highly unlikely to happen and not worth it considering the additional complexity etc..

[derkbell]

Ah yeah, when thinking about this and writing the first part I assumed one would always map from "unknown error message or Error type" into the error type as specified in the outer function signature, probably because that's kind of default in Rust, as you almost always combine the map_err with the ? immediately return operator, like result.map_err(|unknown_err| { MyErrorType() })?

The different error types do make it ugly/complex. Perhaps the mapError method could enforce that the return type of the transformFn parameter matches the outer error type, or would that be too specific and block other use cases of the method?

[derkbell]

The extra paramter in mapCatching seems like a low impact solution, I'll try to add it and see if that works

[derkbell]

I've added an errorTransform parameter to mapCatching. It leverages on the existing functionality in Result.try. Just going to add a mapError to the ResultAsync and then I'll create a PR and you can see if it is fitting

[everweij]

Perhaps the mapError method could enforce that the return type of the transformFn parameter matches the outer error type, or would that be too specific and block other use cases of the method?

Tbh, I don't know Rust in-depth, so I may have misunderstood, but yeah it seems that Rust's behavior deviates from TS here. I do think your suggestion is perhaps a bit too specific indeed. Right now we have a lot of flexibility in how the consuming user structures their errors (by design), so it's tricky to make certain assumptions here. Also, it could be hard to make concrete (both in types and logically) what an 'unknown' error exactly is.
My preference would be to keep it as simple as possible -> mapError transforms Result<Value, ErrA | ErrB | ErrC> into Result<Value, NewError>. I guess you could always utilize explicit function signatures to enforce the exact error-types you permit.

I've added an errorTransform parameter to mapCatching. It leverages on the existing functionality in Result.try. Just going to add a mapError to the ResultAsync and then I'll create a PR and you can see if it is fitting

Cool, thanks!

[derkbell]

I made a PR to add both the mapError methods and extend the mapCatching. I think the mapCatching is quite trivial, I'm not sure how great that error type merging stuff is going to be with the mapError methods. I only added tests to get to 100% coverage, copying the tests from the regular map method.

(There is one other function not tested: an rejected promise from a mapError should not be consumed. This works correctly but I was fighting with vitest to get this properly tested because it saw the unhandled rejection as a faulty test)

[everweij]

Thanks a lot! I've just reviewed your PR, let's continue there :)

[everweij]

Fixed in #9