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

Option.mapEffect #3142

Closed
jamesbirtles opened this issue Jul 2, 2024 · 10 comments · May be fixed by #4362
Closed

Option.mapEffect #3142

jamesbirtles opened this issue Jul 2, 2024 · 10 comments · May be fixed by #4362
Labels
enhancement New feature or request

Comments

@jamesbirtles
Copy link

jamesbirtles commented Jul 2, 2024

What is the problem this feature would solve?

Given an Option<A> and a fn (a: A) => Effect<B, E, R> it would be useful to have a function that could apply that fn to the Some value and return an Effect<Option<B>, E, R> rather than an Option<Effect<B, E, R>> as Option.map would so that it can more easily be used in an effect pipeline.

This is a pattern I am coming across a few times when, for example, decoding from a database query where the potential for a missing row is represented as an Option

What is the feature you are proposing to solve the problem?

Option.mapEffect seems like a fairly obvious api for this I think, I have made my own function to do this:

import { Effect, Option } from 'effect';
import { dual } from 'effect/Function';

export const mapEffect: {
	<B, A, E, R>(
		f: (a: A) => Effect.Effect<B, E, R>,
	): (self: Option.Option<A>) => Effect.Effect<Option.Option<B>, E, R>;
	<B, A, E, R>(
		self: Option.Option<A>,
		f: (a: A) => Effect.Effect<B, E, R>,
	): Effect.Effect<Option.Option<B>, E, R>;
} = dual(2, <B, A, E, R>(self: Option.Option<A>, f: (a: A) => Effect.Effect<B, E, R>) =>
	Option.match(self, {
		onSome: (value) => Effect.map(f(value), Option.some),
		onNone: () => Effect.succeedNone,
	}),
);

What alternatives have you considered?

  • Using my own wrapper as posted
  • Using no wrapper and just writing out the relatively verbose Effect.andThen(Option.match( ... ))
  • A more generic 'transpose' function (as rust calls it) that would take Option<Effect<A, E, R>> and turn it into Effect<Option<A>, E, R>. Then you could use the existing option map in a pipeline like: (..., Effect.map(Option.map(fn)), Effect.andThen(Option.transpose). I think this would be super useful and am slightly surprised I couldn't find a similar function in effect.
@jamesbirtles jamesbirtles added the enhancement New feature or request label Jul 2, 2024
@IMax153
Copy link
Member

IMax153 commented Jul 3, 2024

@jamesbirtles - thank you for opening the issue and providing a very detailed description of the use case!

Given that Option is a subtype of Effect, you can directly use Effect.map to map an Option<A> to get a Effect<B, NoSuchElementException>. Then you can just use Effect.optionFromOptional to convert back to an Effect<Option<B>>. See this playground: https://effect.website/play#149ce22bbd4f.

Going to close this issue as the requested functionality essentially already exists in the library and it is unlikely that we will add a mapEffect to Option.

@IMax153 IMax153 closed this as completed Jul 3, 2024
@IMax153 IMax153 closed this as not planned Won't fix, can't repro, duplicate, stale Jul 3, 2024
@decoursin
Copy link

+1 this seems obvious to me. The name optionFromOptional is unintuitive, and it should be on Option not Effect.

@jamesbirtles
Copy link
Author

i must admit I am also not super satisfied with the optionFromOptional solution proposed by @IMax153.

I very often end up in a scenario like this:

// I have some effect of type `Effect.Effect<Option.Option<A>, E, R>`;
const effect = Effect.succeedSome(1);
// Oh now I want to simply map the `A`, guess I'm doing this rigmarole again
effect.pipe(
  // Flatten to `Effect.Effect<A, E | Cause.NoSuchElementException, R>`
  Effect.flatten,
  // Do the thing I actually wanted to do (imagining this was some operation that required it to be an effect)
  Effect.andThen((x) => Effect.succeed(x + 1)),
  // now back to what I actually want, I hope there's no other `Cause.NoSuchElementException` I'm accidentally capturing here
  Effect.optionFromOptional
);

given that Effect is clearly not adverse to having technically duplicate functionality but with nicer ergonomics based on the million and one functions it already has, I am somewhat surprised that there is a lack of desire to make this (imo very common) use case more ergonomic and discoverable

@IMax153
Copy link
Member

IMax153 commented Oct 27, 2024

@jamesbirtles - in the past, we did have an Option.mapEffect, but adding these types of methods creates multiple issues (which is the reason they were removed several years ago).

Having Effect-based methods in Option also meant that a user had to import all of Effect to use Option (yes, tree-shaking should solve this in most cases, but accidentally using Option.mapEffect would substantially affect bundle size).

But IIRC, the main reason we removed these types of methods was that it was very difficult to draw the line for where to stop adding them, so we opted for providing the functionality within the Effect module instead.

Having said the above, I do agree that we can probably do better on naming in this particular case.

I'll re-open the issue and summon @mikearnaldi, @gcanti, and @tim-smart to further discuss.

@IMax153 IMax153 reopened this Oct 27, 2024
@decoursin
Copy link

I would like to just add my 2 c. Imo, you have to think about target audience, general direction, and convenience. I don't think bundle size is that important to most Effect users. Second, Effect has the task of becoming popular, and that's not going to happen by worrying about pennies of bundle size, but by building libraries that are convenient and useful. But I'm just giving my 2 c, I'm a just a little guy here. I'm very thankful for everything you're building and built either way :)

@gcanti
Copy link
Contributor

gcanti commented Oct 28, 2024

I'm not sure if "mapEffect" is a good name since there are multiple mapEffect functions in Effect, all of which return the same data type (e.g., Stream.mapEffect, Fiber.mapEffect, Sink.mapEffect) while here we start with an Option and we end up with an Effect.

Swapping data types inside out is commonly named as follows:

  • traverse: (Option<A>, A => Effect<B>) => Effect<Option<B>>
  • sequence: (Option<Effect<A>>) => Effect<Option<A>> (transpose is nice though)

p.s.
I would also suggest renaming (i.e., deprecate the old and add the new) optionFromOptional to something more intuitive, like catchNoSuchElementException

@decoursin
Copy link

@gcanti that's sort of the direction that @jamesbirtles suggested:

A more generic 'transpose' function (as rust calls it) that would take Option<Effect<A, E, R>> and turn it into Effect<Option, E, R>. Then you could use the existing option map in a pipeline like: (..., Effect.andThen(Option.map(fn)), Effect.andThen(Option.transpose). I think this would be super useful and am slightly surprised I couldn't find a similar function in effect.

I'm +1 for Option.transpose. I know that traverse and sequence are used in cats effect, but I think transpose is much more intuitive because it literally is a transposition of two types.

@titouancreach
Copy link
Contributor

This is my version of transpose if want to copy past in your projects:

export const transpose = <A, E, R>(
  param: Option.Option<Effect.Effect<A, E, R>>,
): Effect.Effect<Option.Option<A>, E, R> => {
  return param.pipe(
    Option.match({
      onNone: () => Effect.succeedNone,
      onSome: Effect.flatMap(Effect.succeedSome),
    }),
  );
};

@gcanti
Copy link
Contributor

gcanti commented Jan 7, 2025

@titouancreach I think adding defaults to type parameters would help handle Option.none()

import { Option, Effect } from "effect"

declare const transpose: <A, E, R>(
  param: Option.Option<Effect.Effect<A, E, R>>
) => Effect.Effect<Option.Option<A>, E, R>

// const x: Effect.Effect<Option.Option<unknown>, unknown, unknown>
const x = transpose(Option.none())

declare const transposeWithDefaults: <A = never, E = never, R = never>(
  param: Option.Option<Effect.Effect<A, E, R>>
) => Effect.Effect<Option.Option<A>, E, R>

// const y: Effect.Effect<Option.Option<never>, never, never>
const y = transposeWithDefaults(Option.none())

@titouancreach
Copy link
Contributor

@gcanti Totally agree ! thanks

gcanti added a commit that referenced this issue Jan 17, 2025
effect-bot pushed a commit that referenced this issue Jan 19, 2025
effect-bot pushed a commit that referenced this issue Jan 21, 2025
gcanti added a commit that referenced this issue Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants