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

Add intoTuple #12

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Add intoTuple #12

merged 3 commits into from
Oct 25, 2022

Conversation

sidiousvic
Copy link
Contributor

@sidiousvic sidiousvic commented Oct 24, 2022

Adds the method intoTuple to Result.

intoTuple returns a tuple [undefined, T], or [Err<E>, undefined] if the result is Err.

In the Err case, in order to allow for an if (err) comparison, the err member is wrapped in Err.

There is a question of whether the Err case tuple should contain an unwrapped error instead of Err<E>. One problem that may arise when returning an unwrapped err value is when is when it is a falsy value. In that case, this pattern would be unreliable:

if (err) {
   // If err is falsy, we will never reach here in the `Err` case.
} else {
   ...
}

I can come up with a couple of approaches:

  • In the error case, return [Err<E>, undefined]. The caller can then call err.unwrapErr() within the if (err) clause (As demonstrated in this PR)
  • Return unwrapped values without default, have the caller narrow err !== undefined.
  • Somehow force the caller to provide a truthy default for the err value

Interested to hear ideas! @traverse1984 Thank you again for your time and feedback.

@sidiousvic sidiousvic force-pushed the intoTuple branch 2 times, most recently from 726af42 to fa6f47f Compare October 24, 2022 01:57
@traverse1984
Copy link
Owner

I think we have to let the user narrow the type in this case.

Using intoTuple (and indeed, Result generally) will be most useful when the E type is always truthy (or easily narrowed).

I think we should use null, rather than undefined. Thoughts?

@sidiousvic
Copy link
Contributor Author

I think we have to let the user narrow the type in this case.

Sounds good, I agree.

I think we should use null, rather than undefined. Thoughts?

I really don't have a strong opinion here, and I see that null is being used elsewhere in the code for similar effect, so it will be good to stay consistent.

Will submit these updates shortly and put the PR up for review.

@sidiousvic
Copy link
Contributor Author

I have amended the commit with the following changes:

  • Return [E, null] in the Err case
  • Use null in favor of undefined
  • Make intoTuple narrow E to a non-null value in a Result<T, E>. Would appreciate your opinion on this one. Otherwise, the following assertion would be true:
expect(Err(null).intoTuple()).to.deep.equal([null, null]);

Which does not seem very useful, especially for the destructuring pattern desired.

@sidiousvic sidiousvic marked this pull request as ready for review October 25, 2022 13:06
Make doc-comment for `intoTuple` consistent.
@traverse1984
Copy link
Owner

traverse1984 commented Oct 25, 2022

There was still a small mistake in the doc-comment which references [Err(1), ...], however I've updated the comment to fit the style of the rest. That is, a basic, typed example of the functionality without an example use-case. I may review whether to include examples in doc-comments in the future, but that'll be an 'all or nothing' decision.

The Exclude<E, null> requirement seems like a sensible inclusion, but I would not like to include it. Even if the E type includes null, it may be possible to discern the result from the other half of the tuple. For example:

const x: Result<User, Error | null> = fetchUser();
const [err, res] = x.intoTuple();

if (res) {
   console.log(`Welcome ${res.username}`);
} else if (err) {
   console.error(err);
} else {
   console.log('User not found');
}

Do I like that pattern? Not really - but if I had decided to use it, I would not expect a library which is otherwise un-opinionated to prevent me from doing so.

@traverse1984 traverse1984 merged commit bfc5056 into traverse1984:master Oct 25, 2022
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.

None yet

2 participants