traverse1984 / oxide.ts

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

Add intoTuple #11

Closed sidiousvic closed 1 year ago

sidiousvic commented 1 year ago

Adds the method intoTuple that was mentioned in #10.

Convert an existing Option/Result into a tuple containing undefined (or a provided falsey value) and T.

More than happy to adjust style and details as needed. Great work on this library! Very happy using it.

traverse1984 commented 1 year ago

I appreciate your effort here, but this code contains mistakes. The biggest one is that intoTuple for Result is not sound. In the method you cast the inner value as T without knowing whether or not that is the case. The tests added do not test for the behaviour in the doc-comments.

Ultimately, the implementation is flawed. If the mistakes above are fixed, you've still only added a function that returns [undefined, T]. The err part of the tuple never bears any relation to the monad being cast, so it's ultimately pointless. It would be better to manually create a tuple if you really needed this structure:

const [err, res] = [undefined, val.into()]; // Like this
const [err, res] = [, val.into()]; // Or like this

As such, I'm going to close this pull request. I'd welcome a new pull request if you are able to produce a sound implementation which addresses these issues. For reference below I have included a sample of the behaviour I would expect from such a method.

Sample Behaviour

The intoTuple should exist only on the Result type. It has no value being on Option. It should work like this:

const x: Result<string, Error> = generateResult();
const [err, res] = x.intoTuple();

if (err) {
   const y: [Error, null] = [err, res];
} else {
   const y: [null, string] = [err, res];
}
sidiousvic commented 1 year ago

@traverse1984 Many thanks for sharing clear and actionable feedback. I will have a PR within the next couple days with another attempt.

thelinuxlich commented 1 year ago

I'd advocate for returning undefined instead of null for res so you can add a default value on destructuring:

const [err, res = "nothing found"] = await generateResult().intoTuple()