Implementing error handling as described in issue #35

[jplikesbikes]

Ethos

• It should not be the concern of flyd to handle exceptions for the user -- any throw should result in a hard failure.
• Silent failures are bad (current way flyd handles Promise.reject)
• Be as unopinionated in implementation as possible
• Be functional in design
• Be as backward compatible as possible with the current api

Concepts

• The stream is of events
• Each stream has a left and a right side (like an Either)
• The right side is the domain objects
• The left side is meta (in most cases errors)
• By default the api operates on the right side

The Api

s is a stream

Setting data s(...) is overloaded

• s(value) is the default case takes a value makes it a right and pushes it down the stream
• s(promise) if the promise resolves pushes a right, otherwise pushes a left
• s(either) pushes down right or left based on either.left either.right
• s.left(value) sets the stream to a left of value

Getting data

• s() get the last right value or throws an exception if there is a left value
• s.left() get the last left value or throws an exception if there is a right value
• s.either() get the last value out as an Either

Checking stream state

• s.isLeft() return true if the stream contains a left value
• s.isRight() return true if the stream contains a right value

Core functions

• .map() works only on rights and ignores lefts
• .mapEither() gets all events as an Either
• .combine() and .merge() stay the same they work on streams
• .ap() works on rights only
• .scan() works on rights only
• .on() works on rights only

The Either implementation

There are no additional dependencies and we have provided a minimal implementation for basic use. If you plan on using .mapAll we recommend overriding the methods in flyd.Either. You can use folktale/data.either for example as shown below.

var DE = require('data.either');
flyd.Either.Right = DE.Right;
flyd.Either.Left = DE.Left;
flyd.Either.isEither = function(obj) { return obj instanceof DE; };
flyd.Either.isRight = function(e) { return e.isRight; };
flyd.Either.getRight = function(e) { return e.value; };
flyd.Either.getLeft = function(e) { return e.value; };

Other functionality

Keeping with the ethos of flyd any further functions like .swap or .onAll should be implemented as modules.

[coveralls]

Coverage remained the same at 100.0% when pulling 61872ce1c6efec0435a6950fcd29816a11704ff5 on jplikesbikes:master into 5753f0eb2b60d035e4f1602620fb78ee61db5c57 on paldepind:master.

[c-dante]

I feel this is an elegant solution to the errors problem.

The core API stays the same and has the same feeling -- stream(value) to post events, map working only on rights and providing mapAll as a way to unambiguously handle all events and errors, as well as allowing modules like errorsToValues a'la Kefir.

It also doesn't overcomplicate the dependency graph created from attaching to streams -- each flyd.stream object is still a single value stream with a companion end stream.

Also, providing an easy way to swap out the Either implementation to suit your needs is excellent.

[c-dante]

Perhaps we should change some of the API for consistency ->

flyd.mapEither // replace mapAll

stream() // throws if stream.isLeft()
stream.left() // throws if stream.isRight()
stream.either() // never throws, returns the Either

Also, +1 for adding isRight for symmetry, even if it's just !isLeft()

[jplikesbikes]

Good suggestion on the .either() and changing .mapAll() to .mapEither()

[coveralls]

Coverage remained the same at 100.0% when pulling a811386f496b3c71828d6f6ab1d8ed608767788b on jplikesbikes:master into 5753f0eb2b60d035e4f1602620fb78ee61db5c57 on paldepind:master.

[ericgj]

I'm confused by this pull request. If I have something on a stream that predictably throws, I would store it on the stream as an Either Error x -- either an error or a success value -- or a promise that resolves to such. The stream then has the value I put on it. The mechanics of mapping over streams, AKA creating dependent streams, does not change whether the value is an Either or anything else.

This pull request as I understand it attempts to "bake in" Either logic into stream mapping, but in the process makes it less predictable. For instance, correct me if I'm wrong, but doesn't this map function mean that the dependent stream has an undefined value if the source stream value isLeft ?

flyd.map = curryN(2, function(f, s) {
  return combine(function(s, self) {
    if (isRight(s.val)) self(f(s()));
  }, [s]);
})

I have not looked at it too closely, but it seems like there is a kind of "safe" mode, where you would always use s.mapEither and s.either, and deal with the eithers as values, vs the default "unsafe" mode, where you worry about streams having undefined values when you map and do error handling or isRight checking on every s() call.

This all may be fixable, but another issue I have with baking in Either logic is that in a lot of cases it isn't needed at all in streams. Sometimes it is all simply mapping pure functions all the way down. For instance, no stream in the Elm architecture needs Either logic. And if you look at their Signal implementation (surely a model for flyd), not an Either in sight.

Granted, we are talking about JS and not Elm here :tired_face: I sense that one of the main motivations for these changes are to make up for deficiencies or ... weirdness ... in JS promises. If that's the case then why not simply (a) wrap promises to always resolve/reject to an Either as @paldepind suggested or (b) use a Task library (e.g. data.task) ?

[jordalgo]

@ericgj When I first saw this PR (yesterday) I got excited at the idea of having Eithers baked into flyd streams but I think I'm now in your camp in that this is probably creating more problems than it's solving and upsetting the current, and very beneficial, simplicity of these streams.

For me, the possible pains come when you first decide to push Eithers into your stream and want to map over the values inside them. There is just a lot of unwrapping to be done and possible side effects depending on how you want to handle the occurrence of a Left.

E.g.

var flyd = require('flyd');
var R = require('ramda');
var S = require('sanctuary');

var count = 0;
var stream1 = flyd.stream();
var times2 = function(x) { return x * 2; }

setInterval(function() {
  stream1(S.Right([count++]));
}, 1000);

setTimeout(function() {
  stream1(S.Left('Big Error!'));
}, 3000);

flyd
  .scan(function(a, v) {
    if (S.isLeft(a)) {
      return a;
    } else if (S.isLeft(v)) {
      return v;
    } else {
      return a.concat(v);
    }
  }, S.Right([0]), stream1)
  .map(R.map(R.map(times2))) // a lot of unpacking here
  .map(console.log.bind(console));

// outputs
Right([0])
Right([0, 0])
Right([0, 0, 2])
Left("Big Error!")
Left("Big Error!")

I also think that maybe for error handling, I should just push a value into the a stream's end stream and listen for that.

Using Eithers for promises also seems perfectly reasonable.

[jplikesbikes]

@jordalgo's main pain point was my inspiration

For me, the possible pains come when you first decide to push Eithers into your stream and want to map over the values inside them. There is just a lot of unwrapping to be done and possible side effects depending on how you want to handle the occurrence of a Left.

This pr may not be the best solution.

First how about allowing the user to override how flyd handles promises. Something like this (maybe at the stream level instead of global)

// this would be how it works now
flyd.promiseResolved = function(s){ function(result) { return s(result); } };
flyd.promiseRejected = function(s){ function(err) {} };

// change where flyd calls .then(s) to .then(flyd.promiseResolved(s), flyd.promiseRejected(s))

// If using Either you could
flyd.promiseResolved = function(s){ function(result) { return s(Either.Right(result)); } };
flyd.promiseRejected = function(s){ function(err) { return s(Either.Left(err)); } };

// If you want to warn on uncaught promises 
flyd.promiseRejected = function(s){ function(err) { return console.warn('uncaught promise rejection', err); } };

To help working with Either maybe we can introduce a liftMonads function to module

// just to illustrate functionality, something along the lines of this curried
function liftMonads2(fn, ma, mb){
    return ma.chain(a){
        return mb.chain(b){
            return fn(a, b);
        }
    }
}

Updating @jordalgo's example

var flyd = require('flyd');
var liftStream = require('flyd/module/lift');
var liftMonad = require('flyd/module/liftMonad');
var liftEither = R.compose(liftMonad, liftStream);

var R = require('ramda');
var S = require('sanctuary');

var count = 0;
var stream1 = flyd.stream();
var times2 = function(x) { return x * 2; }

setInterval(function() {
  stream1(S.Right([count++]));
}, 1000);

setTimeout(function() {
  stream1(S.Left('Big Error!'));
}, 3000);

flyd
  .scan(liftEither(function(a, b){
    return a.concat(b);
  }), S.Right([0]), stream1)
  .map(liftEither(times2)) // less unpacking here
  .map(console.log.bind(console));

// outputs
Right([0])
Right([0, 1])
Right([0, 1, 2])
Left("Big Error!")
Left("Big Error!")

[jordalgo]

@jplikesbikes I think liftMonads as module would definitely be useful (not being part of the core lib seems risk free) :+1:

As far as the promise overriding, part of me wants to remove any special handling of promises from flyd all together. The fact that it swallow rejections is not good, which is probably why I haven't used the built in promise handling yet. It's functionality that flyd shouldn't have to concern itself with. It might be a fun experiment to try implementing a flyd-like library that only emits Eithers and also try/catches on every single function (map, scan, transduce, etc...) so that if any one of those throws, the stream would stay intact but just emit a Left with the captured error. e.g.

var count = 0;
var stream1 = flyd.stream();

setInterval(function() {
  if (count == 3) {
    stream1('str');
  } else {
    stream1(count++);
  }
}, 1000);

stream1
  .map(function(a) {
    return JSON.parse(a)
  })
 .map(console.log.bind(console));

//Output
Right(0);
Right(1);
Right(2);
Right(3);
Left('SyntaxError: Unexpected token s(…)');

[jordalgo]

Ok, someone tell me what is wrong with this because I'm kinda loving it right now. Here is the modified flyd.map to return Eithers.

var S = require('sanctuary');
function map(f, s) {
  return combine(function(s, self) {
    if (s.val instanceof S.Either) {
      try {
        self(s.val.map(f));
      } catch (e) {
        self(S.Left(e.message));
      }
      return;
    }
    self(S.encaseEither(R.prop('message'), f, s.val));
  }, [s]);
}

Here you can push any value into the stream and it will turn it into an Either and apply the functions safely to the wrapped value - retuning a Left with an error message if any of those applied functions fail.

[paldepind]

First of all, this pull request is high quality. I really appreciate the great and well thought out explanation. Talking about errors is necessary and I am very happy that several people have offered valuable input.

My first reaction when reading the PR was very similar to @ericgj's. One of the powerful ideas in functional programming is that of creating specific solutions to specific problems. These can then be composed together to solve more complex problems.

A constant challenge is to find the right and most useful abstractions. I'm not sure if builting Either into flyd is the right way to do it.

But as both @jplikesbikes and @jordalgo rightly points out the API should still be convenient. I think we should explore how convenient we can make working with Eithers without building support for them into the flyd core.

Let's consider @jordalgo's example. We can make the scan part much simpler by noticing that Either is an applicative. Thus we can use R.lift.

// ... unchanged

R.pipe(
  flyd.scan(R.lift(R.concat)),
  flyd.map(R.map(R.map(times2))), // still a lot of unpacking here
  flyd.map(console.log.bind(console))
)(stream1);

// unchanged ...

We still have the triple map though.

I think @jplikesbikes's liftMonads is a great idea as well. It could also be implemented like this I think:

var liftM = R.compose(R.compose(R.join(id)), R.lift)

But this problem is really an instance of the more general problem of working with a monad (Either) inside a monad (Stream).

The main issue I see with the approach taken in this PR is that it builds two different cases into the same API. s(value) does different things dependend on whether value is an Either or not. The same is the case for flyd.map, flyd.on, flyd.scan, flyd.ap. All these functions now contain special casing for when the stream contains an Either. This special casing would also have to be built in to most flyd modules (unless I'm mistaken). This increases complexity. It also makes it impossible to send use Either with flyd without the automatic unwrapping.

As far as the promise overriding, part of me wants to remove any special handling of promises from flyd all together. The fact that it swallow rejections is not good, which is probably why I haven't used the built in promise handling yet. It's functionality that flyd shouldn't have to concern itself with.

I agree with this. I regret building promise handling into flyd. It seemed like a neat feature at the time and still kinda is. But seperating the functionality into a helper function would be a good idea I think.

@jordalgo, your map function is cool. One problem is that it assumes that f whould report errors by throwing. But if you're using Either it's more likely that f whould be a function that returns an Either.

[jordalgo]

Agreed. This PR is awesome and thoughtful but should not be merged.

@jordalgo, your map function is cool. One problem is that it assumes that f would report errors by throwing. But if you're using Either it's more likely that f would be a function that returns an Either.

@paldepind I'm just tinkering with the idea of a flyd fork that always returns an Either -- maybe "either-flyd" ? (Has there ever been a bad monad joke?)

I'm not sure that's a problem (if I'm understanding correctly) with the map. Users of this hypothetical stream lib would be instructed to write map functions on their streams that don't return Eithers because the error catching is already handled for them. However, if their map function did return an Either, it would just be wrapped inside another Either, which is totally fine; the user would just have to do more unpacking.

[StreetStrider]

This special casing would also have to be built in to most flyd modules (unless I'm mistaken). This increases complexity.

I'm worried about this part most of all. I also expect more hairy name variants occur, like map/mapEither for working with left, right, either/non-either. From my POV this is code smell Number One of doing something wrong, telling that something breaks DRY and «Demetra's Law».