Implement bifunctor

[briancavalier]

Here's a first go. There are several ways to do it. As two examples, we could:

1. ditch the current map implementation entirely and just derive it from bimap, or
2. subclass Bimap from Map, somewhat literally treating bifunctor as a subclass of functor and

I went with 2 initially, but definitely open to discussion.

Todo

• More unit tests
• Docs

Close #33

[coveralls]

Coverage decreased (-0.1%) to 99.866% when pulling e92261d on add-bifunctor into 67f1ac1 on master.

[bergus]

Merge this before or after #20?

[davidchase]

this looks really neat :)

[coveralls]

Coverage remained the same at 100.0% when pulling 04d16fd on add-bifunctor into a006e6e on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 329c354 on add-bifunctor into a006e6e on master.

[briancavalier]

Hmmm, so I think this is actually backwards. Currently, bimap is implemented like bimap(mapFulfilled, mapRejected), such that the "left" function is applied to a fulfilled, i.e. successful, value. That's reversed from the typical convention around an Either, for example, where the right value is typically considered to represent "success".

That raises an interesting potential disparity: We have to choose whether the argument ordering of bimap should be more like bimap for an Either, or should be more like then. I'm inclined to go with Either's argument ordering since bimap is following Fantasy-Land, and then is following ES.

[briancavalier]

As another data point, Fluture uses the Either argument ordering.

[briancavalier]

LOL, and funnily enough, I wrote the bimap docs to use Either-ordering, even though the code used then-ordering.

Ok, I'm convinced ... going with Either-ordering.

[coveralls]

Coverage remained the same at 100.0% when pulling cbd6cd2 on add-bifunctor into a006e6e on master.

[coveralls]

Coverage remained the same at 100.0% when pulling eb1a904 on add-bifunctor into a006e6e on master.

[bergus]

At the expense of everyone's confusion, we also could do the same thing as for ap - make .bimap work like .then, but let fantasyland/bimap work like Either.
Whenever I'm using methods, I'm in an OOP mindset and compare the call to then;
when I'm using function (Ramda.bimap) I'm functionally minded and think of the Either-like Promise type, where the first argument obviously has to map over the error (because I expect to be able to partially apply it).

[davidchase]

me personally if i see bimap method on a promise i personally expect it to work like Either each and every time... in contrast if I am coming from a different background of not FP
then i would probably not make any assumptions about how the bimap works and just go with the API implementation however if I for some reason go to another library such as Task or a Future
I would expect the API to match if they both implement the same method.

Again these are just my personal takes on the subject.

However I think we can all agree that promises are just async Eithers 😛

[briancavalier]

make .bimap work like .then, but let fantasyland/bimap work like Either

It's certainly worth considering.

My thinking right now is that, since we don't have a backward compatibility problem in this case, I prefer calling then the "outlier" and stick with the "typical" FP arg ordering. Bimap is already different from then, so trying to make it more similar may actually cause more confusion. It's really hard to say for sure what'll happen tho :)

Maybe that lack of clarity is a reason just to pick something and do it, calling it v1.1.0, seeing how people react, and adjusting course for v2.0.0 as necessary.

Whenever I'm using methods, I'm in an OOP mindset and compare the call to then; when I'm using function (Ramda.bimap) I'm functionally minded

Yeah, I agree this is the essence of the problem. People who have learned about ES or A+ promises first will know then and will have formed expectations around argument ordering from that. People who are coming to creed with an FP background will know Either (and the like) and will have formed expectations around argument ordering from that.

another library such as Task or a Future I would expect the API to match

That's pretty compelling, imho.

---

Another potential course is simply not to provide Promise.prototype.bimap at all, and provide only Promise.prototype[fl.bimap]. That seems more radical, although I wonder if anyone will actually use Promise.prototype.bimap?

[bergus]

The major use case I can see for .bimap is mapping over errors, without needing to throw … or return reject(…) in a .catch handler. The other callback (for the success case) isn't that interesting.
Which leads to the thought… why not just provide a mapError method only?

Calling .bimap(onerror, onsuccess) is exactly equivalent to .map(onsuccess).mapError(onerror) or .mapError(onerror).map(onsuccess), just with more confusion about argument order (but also one promise and one handler less instantiated, so a little more efficient).
Combine that with a [fl.bimap] method and everyone should be happy.

[briancavalier]

How about this: let's decouple the decision of how to expose error mapping functionality from the decision to add FL 2.0 bifunctor. That means merging #86 first, then rebasing this PR and changing it to provide only the namespaced bimap. Then, we can continue to discuss the best first-class API(s) for bimap, mapError, etc.

[coveralls]

Coverage remained the same at 100.0% when pulling 9dad512 on add-bifunctor into a006e6e on master.

[briancavalier]

Finally unwinding the PR stack and getting back to this. Just rebased, and will prune this back to just implementing the fantasy-land namespaced bimap shortly.

[coveralls]

Coverage remained the same at 100.0% when pulling ec0164d on add-bifunctor into 458d790 on master.

[briancavalier]

Calling .bimap(onerror, onsuccess) is exactly equivalent to .map(onsuccess).mapError(onerror)

They aren't exactly equivalent: if onsuccess throws, onerror will be called in the case of .map(onsuccess).mapError(onerror), but not in the case of bimap(onerror, onsuccess). It's true in the reverse order, .mapError(onerror).map(onsuccess), which is interesting because onerror appears first in that and in bimap.

I'm not sure if or how that helps us in making a decision about mapError and public bimap, tho.

[briancavalier]

Ok, 67c072a includes only a FL 2.0 namespaced bimap. The travis build fails because of jsinspect, annoyingly. If I bump up the threshold by 1 to 36, it passes :/

My mood right now on bimap is that it's not harmful to expose it "publicly", i.e. non-namespaced, as long as we take care to document it as clearly as we can, with good examples. That also certainly doesn't rule out an additional mapError.

[coveralls]

Coverage remained the same at 100.0% when pulling 216fa42 on add-bifunctor into f005967 on master.