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

Functor: add bindMap function as a do-notation helper #1725

Merged
merged 5 commits into from
Sep 14, 2022

Conversation

Punie
Copy link
Contributor

@Punie Punie commented Jun 14, 2022

Hello 👋

I took the liberty to create a new function on the Functor typeclass to help with introducing new values in do-notation context while leveraging pure computations that depend on said context.

ie. I needed to do something akin to this (example greatly simplified from my original genuine use-case):

pipe(
  option.Do,
  option.apS('x', some(42)),
  option.apS('y', some(1337)),
  option.bind('z', ({ x, y }) => option.of(x * y)),
); // => Option<{ x: number; y: number; z: number }>

Another way to achieve the same result would be to just use option.map at the last step:

pipe(
  option.Do,
  option.apS('x', some(42)),
  option.apS('y', some(1337)),
  option.map(factors => ({
    z: factors.x * factors*y,
    ...factors,
  })),
); // => Option<{ x: number; y: number; z: number }>

I find both these solutions unsatisfying so I introduced the bindMap function on Functor specifically for those use-cases. It is to map what bind is to chain and apS is to ap.

With this, we can rewrite the example above as such (getting rid of the option.of or the recreation of the context object):

pipe(
 option.Do,
 option.apS('x', some(42)),
 option.apS('y', some(1337)),
 option.bindMap('z', ({ x, y }) => x * y),
); // => Option<{ x: number; y: number; z: number }>

I hope this can be of use to others as well. I'm not too fond of the name bindMap but it's the best I could think of. I kinda liked let but as a reserved keyword in TS it was a no-go.

@imcotton
Copy link
Contributor

previously #1524?

@samhh
Copy link
Contributor

samhh commented Jun 15, 2022

@imcotton I think you're right that this covers that use case as well.

For my mental model, I think this PR is equivalent to:

do
  x <- Just 42
  y <- Just 1337
  let z = x * y -- this line
  etc

@Punie
Copy link
Contributor Author

Punie commented Jun 15, 2022

@imcotton oh you're right, I failed to notice that there was already an open issue on this.

It does cover this use case and yes that is more or less the let in Haskell's do-notation.
The bindMap naming is not too satisfying but let being a reserved keyword, it was the best I could find. Open to suggestions on naming though.

@imcotton
Copy link
Contributor

imcotton commented Jun 15, 2022

One thing I noticed from your implementation is allowing redefine existing keys, e.g.

pipe(
 option.Do,
 option.apS('x', some(42)),
 option.apS('y', some(1337)),
 option.bindMap('y', ({ x, y }) => x * y),  // <-- reassign y here
)

Is this intend behavior or oversight in compare to:

fp-ts/src/Chain.ts

Lines 156 to 163 in 34b28fa

export function bind<M>(
M: Chain<M>
): <N extends string, A, B>(
name: Exclude<N, keyof A>,
f: (a: A) => HKT<M, B>
) => (ma: HKT<M, A>) => HKT<M, { readonly [K in keyof A | N]: K extends keyof A ? A[K] : B }> {
return (name, f) => (ma) => M.chain(ma, (a) => M.map(f(a), (b) => Object.assign({}, a, { [name]: b }) as any))
}

in L159: name: Exclude<N, keyof A>?

@Punie
Copy link
Contributor Author

Punie commented Jun 21, 2022

@imcotton I'll be honest, that is an oversight on my part and I will fix it for consistency with bind and apS.
However, I'd argue that this restriction is unnecessary for all of these methods as shadowing is a thing in "regular" do-notation.

Good catch, though. Thank you 🙏

@imcotton
Copy link
Contributor

However, I'd argue that this restriction is unnecessary for all of these methods as shadowing is a thing in "regular" do-notation.

IMO, name shadowing from outer scope is reasonable (but I'd avoid it personally), however in case of let bind in do notation probably a sign of something off.

Overall I'm 👍 for this feature tho, thanks for the effort.

@Punie
Copy link
Contributor Author

Punie commented Jun 22, 2022

@imcotton I agree and I did make the change again for consistency with bind and apS.

I still think that although it is a smell in most cases, it can be desirable in some to override a value with a modified one but at the end of the day it is not the subject of this PR (hence the change 😄).

Thank you for the find and the support 🙏

@gcanti gcanti added this to the 2.13 milestone Aug 31, 2022
@gcanti
Copy link
Owner

gcanti commented Aug 31, 2022

For what concerns the name I'd really like let (*), ...or maybe bindValue (<- already used by effect-ts)

(*)

as a reserved keyword in TS it was a no-go

const let_ = ...

export {
  /**
   * @since 2.13.0
   */
  let_ as let
}

is it a viable option?

@Punie
Copy link
Contributor Author

Punie commented Sep 14, 2022

@gcanti thank you for the suggestion, I was really bummed I couldn't use let as I really liked it as well and it turns out I could! 🎉
I didn't know TS would allow that but it would seem it does happily.

I changed the name to let then 👍

Let me know if something is missing / needs changing in the PR

@gcanti
Copy link
Owner

gcanti commented Sep 14, 2022

@Punie some N extends string are missing I think

@gcanti
Copy link
Owner

gcanti commented Sep 14, 2022

Never mind, I was looking at the md files :)

@gcanti gcanti merged commit cd06390 into gcanti:master Sep 14, 2022
@gcanti
Copy link
Owner

gcanti commented Sep 14, 2022

Thank you!

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.

4 participants