Skip to content

Conversation

@boyeln
Copy link

@boyeln boyeln commented Sep 12, 2024

As discussed in #567 (comment), the current type signature of fromThrowable allows users to pass a specific error value type, without providing an error mapping function that produces the provided error type:

const safeParse = Result.fromThrowable<typeof JSON.parse, number>(JSON.parse);
      // ^ (text: string, reviver?: (this: any, key: string, value: any) => any) => Result<any, number>

This PR tries to solve that by adding some overloads to the function, only allowing users to specify the error type when a matching error mapping function is provided.

@changeset-bot
Copy link

changeset-bot bot commented Sep 12, 2024

🦋 Changeset detected

Latest commit: 2815b6f

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

@@ -0,0 +1,5 @@
---
'neverthrow': minor
Copy link
Author

Choose a reason for hiding this comment

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

This might be considered a breaking change, since users that use the function like described in the PR will get red squiggles. However, I would argue that this is a "type bug", and shouldn't be considered a valid use case

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 agree to consider this a bug fix

Comment on lines +25 to +31
/**
* Wraps a function with a try catch, creating a new function with the same
* arguments but returning `Ok` if successful, `Err` if the function throws
*
* @param fn function to wrap with ok on success or err on failure
* @param errorFn when an error is thrown, this will wrap the error result
*/
Copy link
Author

Choose a reason for hiding this comment

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

It's not the most elegant to have to maintain two versions of the documentation, however I'm not aware of any other way of doing it. Please let me know if there's a better way!

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't avoid that, I guess.
Here is the official doc comment of Array.prorotype.concat. The same content is written in two places.
image

Copy link
Collaborator

@m-shaka m-shaka left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you
Sorry for the late review

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.

4 participants