traverse1984 / oxide.ts

Rust's Option<T> and Result<T, E>, implemented for TypeScript.
MIT License
517 stars 19 forks source link

Add intoTuple #12

Closed sidiousvic closed 2 years ago

sidiousvic commented 2 years ago

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:

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

traverse1984 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I have amended the commit with the following changes:

expect(Err(null).intoTuple()).to.deep.equal([null, null]);

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

traverse1984 commented 2 years ago

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.