boxed icon indicating copy to clipboard operation
boxed copied to clipboard

Add `or` and `orElse` method to Option

Open acadeau opened this issue 2 years ago • 8 comments

Hello,

I've made this little change to add or and orElse functions to Option type.

The goal is to have a function which will update the value only if we didn't have it before.

The orElse function is the lazy version of or.

Feel free to comment it.

Thank you.

Inspired by: Rust: Option

acadeau avatar Apr 09 '22 14:04 acadeau

Hi! Thank you for your PR!

I'm a bit unsure about that API as I haven't seen much need for that pattern in the last few years. One of the main goals of that project is to reduce the mental overhead approaching these data-structures, and we really need to feel confident that the cost on API surface is worth adding a feature.

Here are the patterns I can identify:

Taking the first Some option out of several

Cases where you have an indeterminate amount of Option<V> and want to take the first with a value. I think that this can be handled user-land.

const firstSome = <A>(options: Array<Option<A>>) =>
  Array.getBy(options, (x) => x.isSome()).flatMap((x) => x);

Falling back to a known value

Having option.or(None()) is a no-op, and option.or(Some(value)) is equivalent to Option.Some(option.getWithDefault(value))

option.getWithDefault(fallback)

Fallback to an indeterminate value

I think that the following piece of code is clearer to understand for such cases:

option.match({
  Some: (value) => Some(value),
  None: () => fallbackOption
})

In general, I think I'm confused by the naming of or and orElse. They don't immediately convey the meaning that they take Option<V> or () => Option<V> as argument, and create confusion with getWithDefault.

I'll leave this open while giving it more thoughts. (cc @zoontek if you want to weight in)

bloodyowl avatar Apr 09 '22 17:04 bloodyowl

Interesting feedback, thank you.

My use case was the following: don’t update the option if there is already a value. This is just to avoid a ternary condition like option.isSome() ? option : optionb. However, I can misuse the api.

I let the PR open to see if it’s worth to have this API.

P.S: or and orElse function names are from rust api that I was familiar with.

acadeau avatar Apr 09 '22 17:04 acadeau

To avoid using a ternary on a single fallback like this, I'd go for the following, which I think transcribes the intent correctly:

option.match({
  Some: (value) => Some(value),
  None: () => optionB
})

bloodyowl avatar Apr 09 '22 18:04 bloodyowl

or/orElse is a really great utility type when dealing with remapping None and is a bit more readable when chaining rather than multiple getWithDefault's as it gets pretty verbose and harder to read:

let maybeNone = Option.None(); // Placeholder for something that may be none
let anotherMaybeNone = Option.None();

// Chaining or
let something = maybeNone.or(anotherMaybeNone).or(Some(exhausted));

// Chaining with getWithDefault
let something = maybeNone.getWithDefault(Some(0)).map(() => anotherMaybeNone).getWithDefault(exhausted);

// Using match
let something = maybeNone.match({
  Some: (value) => value,
  None: () => anotherMaybeNone.getWithDefault(exhausted),
})

greym0uth avatar May 16 '22 18:05 greym0uth

I'm thinking of a possible unified API between structures, following the familiar Promise.race API

// first to resolve
Future.race([a, b, c])
// first in array to be filled
Option.race([a, b, c])
Result.race([a, b, c])
AsyncData.race([a, b, c])

Alternatively, naming it first for Option, Result and AsyncData might disambiguate a bit between the two behaviors.

What do you think?

bloodyowl avatar May 16 '22 18:05 bloodyowl

Hmm a tough one as I think both are a good solution and do the same thing but just read differently. Since the focus of this lib is around chaining I would side with the chaining method more, but race is more common in JS.

orElse is a case to cover though as its a function that returns:

Option.race([maybeNone, () => anotherMaybeNone, Some(exhaused)]);

maybeNone.orElse(() => anotherMaybeNone).or(Some(exhausted));

Now that im thinking about this more too its more similar to the mapError in the result type so maybe this could just be a mapNone addition or could open up the debate of renaming mapError to or/orElse

greym0uth avatar May 16 '22 18:05 greym0uth

@greym0uth could you provide real-world examples where chaining would make more sense here? As said earlier in the thread, that's a pattern I've seen very low usage for in recent years.

bloodyowl avatar May 17 '22 08:05 bloodyowl

The only I can think of that I didn't already put in an example is querying something that returns Option<Something> and if its None flat mapping it to another query that returns Option<Something> for any number times:

function queryOne(): Option<Something> { /... }
function queryTwo(): Option<Something> { /... }
function queryThree(): Option<Something> { /... }
function queryFour(): Option<Something> { /... }

// Doing:
// Using `or`
let exhaustsSome = queryOne().or(queryThree()).or(queryFour());

// Using `orElse`
let exhaustsAll = queryOne().orElse(queryTwo).orElse(queryThree).orElse(queryFour);
let exhaustsSome = queryOne().orElse(queryThree).orElse(queryFour);
let exhaustsSomeBackwards queryFour().orElse(queryThree).orElse(queryTwo);


// Vs using whats currently available
// Using `getWithDefault`
let exhaustsAll = Option.fromUndefined(
  queryOne().getWithDefault(Some(
    queryTwo().getWithDefault(Some(
      queryThree().getWithDefault(Some(
        queryFour().getWithDefault(undefined))))))));
// getWithDefault expects `Something` as a parameter; it 
// isnt possible to end with an option in this method, so
// we have to use `fromUndefined()`.


// Using `match`
let exhaustsAll = queryOne()
  .match({
    Some: (value) => Some(value),
    None: () => queryTwo(),
  })
  .match({
    Some: (value) => Some(value),
    None: () => queryThree(),
  })
  .match({
    Some: (value) => Some(value),
    None: () => queryFour(),
  });

Honestly no matter the API (using .race or using .or/.orElse) I think having a simple way to map a none functionally should be available and is a good utility and currently there isn't a real clean way to map a Option.None without using .match (very verbose if I only care about the None) or Some(option.getWithDefault()) (breaks a functional chain).

greym0uth avatar May 17 '22 22:05 greym0uth

Starting from 0.12.1, you now have Array.keepMapOne to achieve this.

The rationale is that in this situation, you need all of the options to hold the same type, therefore it seems a bit more logical and easier to reason about to think about this with an array.

Array.keepMapOne([Option.None(), Option.Some(1), Option.Some(2), Option.Some(3)], x => x)
// Option.Some(1)

// doesn't evaluate any function after the first one returning an Option.Some(...)
Array.keepMapOne([queryOne, queryTwo, queryThree, queryFour], f => f())

Hope it suits your needs!

bloodyowl avatar Feb 21 '23 11:02 bloodyowl

Yes thanks for this update!

acadeau avatar Feb 22 '23 09:02 acadeau