Refactor expectMap/flatMap/map into one function · Pull Request #13 · tentwentyfour/lonad

ghost · 2019-12-03T10:11:29Z

This diff proposes to converge expectMap / flatMap / map into one function and creating aliases for them.

Summary: - usage of `map`, `flatMap`, `expectMap` and their internal use of `transformResult` and `expect` were hard to grasp - merged `map`, `flatMap` and `expectMap` into a new applicator `chain` able to handle nil values, raw values, results, optionals, promises - it is like `map` except it also prevents otherwise resulting nested objects and supports more types - IMHO `chain` is a good naming choice because: - it enables the developer to distinguish between js arrays and lonad results (searchable) - it is also known as flatmap or bind in other languages (relatable) Test Plan: npm test Reviewers: kwisatz, lattam, paul Subscribers: max, elia, federico Differential Revision: https://cator.1024.lu/D1922

floriansimon1 · 2019-12-03T11:45:29Z

    return this.value;
  },

-  map(λ) {


We need to preserve compat for this which users of a monad-like will expect there to exist, so we can add but not remove

Additionally, I like to have something that is clear and predictable instead of something magical (which more often than not is unpredictable!)

Well, I thought map is redundant in monads. flatMap already does that

map and flatMap are supposed to be different in monads.

For any monad Monad:
map (or fmap) takes a function which has the signature: a => b
whereas
flatMap (or bind, chain, or (>>=)) takes a function with the signature a => Monad b

floriansimon1 · 2019-12-03T11:45:46Z

-    return transformResult(() => λ(this.value), Ok);
-  },
-
-  flatMap(λ) {


Same as above

that's why I chose to have aliases

flatMap is not supposed to do any trick, and the management of Promises is already more than it should do

floriansimon1 · 2019-12-03T11:49:11Z

-  flatMap(λ) {
-    return transformResult(() => λ(this.value), Result.expect);
+  chain(λ) {
+    try {


DecideWrapper should encapsulate all of the logic to transform values. I see why you need that, but it's a bit weird to split the logic in two places (the try catch and decideWrapper)

floriansimon1

Not gonna lie, this feels like another do-all method to rule them all, with its own unpredictable choices. That was the goal of expectMap and it's been a miserable failure. IMO, we should go towards less magic and more explicitness to make methods rememberable and the code easier to debug. Feel free to improve on expectMap, and rename it, but don't make it even less predictable.

https://xkcd.com/927/

floriansimon1 · 2019-12-03T11:51:02Z

-
-  flatMap(λ) {
-    return transformResult(() => λ(this.value), Result.expect);
+  chain(λ) {


Not a big fan of the name tbh. Chain is traditionally an alias for bind/flatMap, and imthis might surprise our users. Why not just repurpose transform?

hm I thought I had called this transform, and created the aliases chain, flatMap and bind

floriansimon1 · 2019-12-03T11:52:16Z

-  flatMap:          returnThis.unary,
  replace:          returnThis.unary,
  property:         returnThis.unary,
-  expectMap:        returnThis.unary,


Good riddance!

floriansimon1 · 2019-12-03T11:53:03Z


-[['transform', 'map']].forEach(([alias, method]) => {
+const decideWrapper = cond([
+  [value => [null, undefined].includes(value), constant(Error())],


Null & undefined are OK values. Why would they be an error?

I think I didn't invent this behavior, not sure, need to investigate

floriansimon1 · 2019-12-03T11:57:55Z

-[['transform', 'map']].forEach(([alias, method]) => {
+const decideWrapper = cond([
+  [value => [null, undefined].includes(value), constant(Error())],
+  [value => typeof value !== 'object',         value => Ok(value)],


Dangerous and unnecessary

ok, I think I also took this one from somewhere, need to investigate

floriansimon1 · 2019-12-03T11:59:03Z

+  [value => isPromise(value),                  value => Result.Pending(value.then(Result.expect, Aborted))],
+  [value => value.isResultInstance,            value => value],
+  [value => value.isOptionalInstance,          value => value.match({ Some: Ok, None: Error })],
+  [constant(true),                             value => Optional.fromNullable(value).match({ Some: Ok, None: Error })]


You can't have nullables at that point so you can just use OK

floriansimon1 · 2019-12-03T12:01:23Z

    return this.value;
  },

-  map(λ) {


Additionally, I like to have something that is clear and predictable instead of something magical (which more often than not is unpredictable!)

ghost · 2019-12-03T16:39:32Z

Not gonna lie, this feels like another do-all method to rule them all, with its own unpredictable choices. That was the goal of expectMap and it's been a miserable failure. IMO, we should go towards less magic and more explicitness to make methods rememberable and the code easier to debug. Feel free to improve on expectMap, and rename it, but don't make it even less predictable.

https://xkcd.com/927/

So you think it's better to have one function which unwraps Optionals, one that unwraps Results, one that unwraps Promises instead of the decideWrapper ? i.e. mapOptional, mapPromise, mapResult, mapRaw, ..

Yes it feels like magic, but one function which is interoperable with all those types is not necessarily unpredictable.

floriansimon1 · 2019-12-03T17:43:14Z

So you think it's better to have one function which unwraps Optionals, one that unwraps Results, one that unwraps Promises instead of the decideWrapper ? i.e. mapOptional, mapPromise, mapResult, mapRaw, ..

As a matter of fact, I do! Nobody complains on arrays that they have to use flatMap instead of map automatically flattening arrays (worse, in the case of arrays it would actually be a detriment!). It also helps when writing typings. It also gives more info at a glance about your code(*). Sure, it's error-prone in Javascript because you can mix them up, but none of that would occur if Result had types.... Which I unfortunately haven't written yet.

No need for mapResult. That's flatMap. mapRaw is just plain map. I'm still currently for abstracting away promises, because it's mostly predictable, but I might change my mind if I ever face inconsistencies.

On the topic of mapOptional: an absent value is not necessarily an Error. I don't think it's unreasonable to have Result<Optional<a>>, especially when writing more generic code. You can have explicit unwrapping but you need an escape hatch if a Result<Optional<a>> is what you need.

Yes it feels like magic, but one function which is interoperable with all those types is not necessarily unpredictable.

expectMap (which is more or less what you're trying to replace) certainly is, and your auto-erroring of null or undefined, or handling of Optionals certainly verge on the unpredictable side. Every time I use expect or expectMap, I have to look at the implementation to know what I will get out, which is to me a sign of bad design on my part.

As I said, if you know how to make it more predictable and easier to use, feel free to change it in any way you see fit (including the name!), but don't change map or flatMap.

(*: Ok(userId).optionalMap(getUser) conveys more info about getUser than Ok(userId).chain(getUser): you know getUser can fail to get a user)

Kevin Ley added 8 commits November 3, 2019 03:56

swap all applicators with new chain

c6c9192

expect may also use the conditional

debb189

refactor

1e841aa

update tests

3aebf17

revert recoverWhen

5a1485a

rename chain to transform

c1db9c7

use transform internally, create aliases flatMap, bind and chain

213d571

ghost requested a review from floriansimon1 December 3, 2019 10:43

floriansimon1 reviewed Dec 3, 2019

View reviewed changes

floriansimon1 requested changes Dec 3, 2019

View reviewed changes

Uh oh!

Uh oh!

Conversation

ghost commented Dec 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floriansimon1 Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floriansimon1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 3, 2019

Uh oh!

floriansimon1 commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

floriansimon1 Dec 3, 2019 •

edited

Loading

floriansimon1 commented Dec 3, 2019 •

edited

Loading