mapError method and mapCatching errorTransform parameter #8

derkbell · 2024-09-02T13:33:24Z

No description provided.

everweij · 2024-09-02T17:48:01Z

src/result.ts

 	 * @returns a new {@linkcode AsyncResult} instance with the transformed value
 	 */
-	mapCatching<ReturnType>(transform: (value: Value) => ReturnType) {
+	mapCatching<ReturnType, ErrorType extends AnyValue>(


Atm, we're not doing anything with the ErrorType in the return type of mapCatching.
We can fallback to NativeError when the error transform-fn is not being used, and use the generic ErrorType in the return type of mapCatching:

Suggested change

mapCatching<ReturnType, ErrorType extends AnyValue>(

mapCatching<ReturnType, ErrorType extends AnyValue = NativeError>(

everweij · 2024-09-02T17:51:33Z

src/result.ts

+				.catch((error: unknown) =>
+					resolve(Result.error(errorTransform ? errorTransform(error) : error)),
+				);


We should probably wrap the body of the catch in an additional try-catch, because the errorTransform function might throw, and atm those exceptions get swallowed. Instead, I think the caught expection should be passed to the missing reject callback of the AsyncResult constructor.

Something along the lines of:

try { resolve( Result.error(transformError ? transformError(error) : error), ); } catch (err) { reject(err); }

everweij · 2024-09-02T17:53:25Z

src/result.ts

+				);
 		}) as ReturnType extends Promise<infer PromiseValue>
 			? PromiseValue extends Result<infer ResultValue, infer ResultError>
 				? AsyncResult<ResultValue, Err | ResultError | NativeError>


Suggestion to replace all ocurrences of NativeError in this return type with ErrorType

everweij · 2024-09-02T17:55:15Z

src/result.ts

 	 * if the transform function is async.
 	 */
-	mapCatching<ReturnType>(transform: (value: Value) => ReturnType) {
+	mapCatching<ReturnType, ErrorType extends AnyValue>(


I think the same comments about the async-variant apply here as well

everweij · 2024-09-02T17:56:23Z

src/result.ts

+			this.success
+				? Result.try(
+						() => transform(this._value),
+						(error: any) => (errorTransform ? errorTransform(error) : error),


If I'm correct, you can pass the reference to errorTransform fn as is, because they both have the same signature.

everweij · 2024-09-02T18:02:08Z

src/result.ts

 	 * in a failed result.
 	 *
 	 * @param transform callback function to transform the value of the result. The callback can be async as well.
+	 * @param errorTransform optional callback function to transform the value of the error. The callback can be async as well.


I don't think we should support an async errorTransform (at least for now), and I don't think this implementation supports it either.

everweij · 2024-09-02T18:12:40Z

src/result.test.ts

 			});
 		});

+		describe("mapError", () => {


I see that you use CustomError a lot in the added test cases. At first glance there is nothing particularly wrong with this, although type-wise it tend to 'slip through the cracks' here and there. Are you ok with using the ErrorA and ErrorB classes to proof things like transformations of errors, etc? As bonus you only have to match against the constructor of the error.

everweij · 2024-09-02T18:15:34Z

src/result.test.ts

+						throw new Error("TEST_ERROR");
+					},
+					(_error) => {
+						return new CustomError();


This is an example of where CustomError creates a false positive.
If you replace this with ErrorA it will probably fail the typecheck. See comment about types of mapCatching in result.ts.

everweij · 2024-09-02T18:16:35Z

src/result.test.ts

+				Result.assertError(result);
+				expect(result.error).toBeInstanceOf(CustomError);
+			});
 		});


Can you add a test for when an exception gets thrown inside the errorTransform fn?

everweij · 2024-09-02T18:18:14Z

src/result.test.ts

 				expect(result.error).toBeInstanceOf(CustomError);
 			});
+
+			it("transforms thrown errors when an errorTransform function is defined", async () => {


I think the same comments of the sync-variant of mapCatching apply here as well

everweij

Thanks for your work, I appreciate it!! 👍🏻
I think in general it looks good. I have a couple of comments, and one bigger concern in general: are there any practical use cases where it makes sense for mapError to accept anything other than a function that returns only another error? I notice that in this PR mapError also accepts the transform fn to be async and/or return another Result, and I'm not sure about that. I could be overlooking something, so please correct me, but in general I like to restrict the API as much as possible, with the reasoning that it is always easier to relax it later on, instead of vice versa.
Let me know what your thoughts are, or if you need any help.

derkbell · 2024-09-02T20:18:15Z

Yeah, it could be simplified. I was just going to describe a particular use case I have/had. The fetch API returns a Response object that can be checked for an ok-ish response (2xx status code) by checking the response.ok boolean. In case of an ok response, I expect response.data to be valid JSON. In case of a 500 error, it could be valid JSON but it could be some raw error message from whatever failed. So in the mapError I have to do a response.json() which is async and could throw as well.

However, I think that could also be handled with a map because map can return a Result. Something like

return await Result.try(
    () => fetch(url, options),
    (error) => MyApplicationError.from(error, url),
).map(async (response) =>
    response.ok ? Result.ok(await response.json()) : Result.error(await handleErrorResponse(response)),
)

handleErrorResponse does await response.json() and if that fails it does await response.text(). It is (response: Response) => Promise<MyApplicationError>

So, long story short, I do have errors that need some async work but it isn't really error mapping, more error construction that can be done in a regular map

derkbell · 2024-09-02T20:20:35Z

On the other notes, thanks for the feedback, this is a nice way to dive into the typescript typesystem. Will go through the feedback whenever I have time this week.

everweij · 2024-09-02T20:41:54Z

I think it's an interesting case you got there. I will give it some further thought, but for now, I agree with your conclusion -> differentiate between error mapping and construction.

I already had a branch locally a couple of days ago that contained some drafts regarding mapError and I couldn't resist working on it some more tonight, so essentially, I just pushed what I've got and added the things that I discussed in the comments (#9).
I know you have put quite some effort into this PR, and I almost don't dare to ask, but are you ok with picking my PR over yours?

derkbell · 2024-09-02T20:59:49Z

Yeah go for it, don't worry, it was a good practice session for me. Learned a lot (and switching to biome for formatting right now)

everweij · 2024-09-03T07:42:50Z

Ok, that's good to hear! Thanks again for your help and your input. Don't hesitate to submit another issue if you need help or have any suggestions to improve this lib.

derkbell and others added 4 commits September 2, 2024 09:53

mapError: sync (Result) part

fbd3d6b

mapCatching: optional errorTransform function

02c2658

mapError: async (AsyncResult) variant

7d754e5

mapError: async (AsyncResult) variant

6471d77

everweij reviewed Sep 2, 2024

View reviewed changes

everweij requested changes Sep 2, 2024

View reviewed changes

everweij closed this Sep 3, 2024

	mapCatching<ReturnType, ErrorType extends AnyValue>(
	mapCatching<ReturnType, ErrorType extends AnyValue = NativeError>(

mapError method and mapCatching errorTransform parameter #8

mapError method and mapCatching errorTransform parameter #8

Uh oh!

Conversation

derkbell commented Sep 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everweij left a comment

Choose a reason for hiding this comment

Uh oh!

derkbell commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derkbell commented Sep 2, 2024

Uh oh!

everweij commented Sep 2, 2024

Uh oh!

derkbell commented Sep 2, 2024

Uh oh!

everweij commented Sep 3, 2024

Uh oh!

Uh oh!

derkbell commented Sep 2, 2024 •

edited

Loading