Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(neon): Implement TryIntoJs/TryFromJs for either::Either #1078

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

kjvalencik
Copy link
Member

This implements the extract traits for either::Either. I chose not to feature flag it because either is a very small crate with zero dependencies.

The Ok implementation paths were very clear. However, there isn't an obviously correct implementation for a failed TryFromJs.

We have two errors, both L::Error and R::Error. This PR does the following:

  • Creates a new Error that prints the type name of L and R saying that they failed
  • Attaches left and right keys to the error object with their TryIntoJs implementation

Another option would be to return only one of the errors. Unfortunately, we can't attach one error to the other because it might not be an Object.

What's the best thing to do for this?

@kjvalencik kjvalencik requested a review from dherman October 3, 2024 16:34
@dherman
Copy link
Collaborator

dherman commented Oct 7, 2024

What's the best thing to do for this?

I agree with what you did. Two errors occurred, so neither should be dropped; the caller needs to decide what to do and should have full access to both. It kind of reminds me of how exception cause chaining preserves a history for introspection.

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Short and sweet!

🚢

@kjvalencik kjvalencik force-pushed the kv/try-from-error branch 2 times, most recently from b548935 to 3721a24 Compare October 7, 2024 20:57
Base automatically changed from kv/try-from-error to main October 7, 2024 21:16
@kjvalencik kjvalencik merged commit 1efcd4e into main Oct 7, 2024
9 checks passed
@kjvalencik kjvalencik deleted the kv/either branch October 7, 2024 21:20
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.

2 participants