paldepind / flyd

The minimalistic but powerful, modular, functional reactive programming library in JavaScript.
MIT License
1.56k stars 85 forks source link

Handle Error cases #35

Closed ybybzj closed 9 years ago

ybybzj commented 9 years ago

Any thoughts on how to handle errors in a stream? For example, if a promise is rejected, from what i saw in code the stream will never be triggered, since the promise only invoke like this n.then(s) //line 134.

I try to improve this, but have not come up a rational solution. Should stream just end itself when it encounters an error, or should it provide a way to be listened to its errors?

ybybzj commented 9 years ago

OK, i didn't see #20. but, still how about promise when it is rejected?

paldepind commented 9 years ago

This is a great question and I'm aware that the documentation doesn't describe errors at all :(

My current opinion is that Flyd should not concern itself with error handling. I don't see any benefit in doing so.

For normal synchronous code developers know how to handle errors. You need to handle that some functions might return null/undefined in certain cases and if you're using functions in a way so they might throw you use try/catch. Promises offers similar features for asynchronous code.

You know all of the above of course. But that Flyd doesn't do any n.catch(someErrHandler) might still seem like a problem to you. This is why I don't think it's a problem. First consider an example with synchronous error handling:

var userStringStream = flyd.stream(); // user information that should be formatted as JSON
var userData = flyd.map(function(userString) {
  try {
    return JSON.parse(userString);
  } catch (err) {
    return emptyUserData;
  }
}, userStringStream);

The above is pretty straight forward. We do an operation that might throw JSON.parse. If it doesn't throw the stream is set to it's return value and if it does some default data is used instead. The same thing can be achieved with promises today:

var userDataUrl = flyd.stream(); // and URL from which user information can be loaded
var userData = flyd.map(function(url) {
  return fetch(url).catch(function(err) {
    return emptyUserData;
  });
});

Is you've properly noticed and mentioned Flyd doesn't handle rejected promises. The idea is that you should never send promises that might reject down a stream. This is achieved by adding catch handlers to your promises before you return them to a stream.

The benefits of the above method is:

This is my current thinking and this is what I've done until know. But this is not set in stone. If you see any problems with the above approach by all means express your concerns!

roobie commented 9 years ago

Would be nice, though, if flyd could tell if user code forgot to make sure that a promise never gets rejected down a stream. I'm thinking p.catch(() => console.warn('flyd doesn\'t handle errors'))

paldepind commented 9 years ago

@roobie I think that is a very good idea!

ybybzj commented 9 years ago

Just an opinion though, the "catch" method of Promise provides a way to handle all the up stream errors in one place, so that you don't need to write catch code to each one of them. If just let stream pass on its error to its dependencies then i can handle all errors in one place, and reduce some code. For instance, when you compose a complex stream with merge or map or some other module methods, then if i can only handle errors in this final stream that would be nice:). Sometimes, you may consume promises from outside that don't catch errors, and you have to wrap a function to each of them to catch errors. For example, I hope i can do this:

var api = require('./api');

var s1 = flyd.stream(api.getA());
var s2 = flyd.stream(api.getB());
var s3 = flyd.stream(api.getC());

var s = flyd.stream([s1, s2, s3], function(self){
    if(flyd.isErr(s1()) || flyd.isErr(s2()) || flyd.isErr(s3())){
         return {};
    }else{
        return {
             s1: s1(),
             s2: s2(),
             s3:s3()
        };
    }
});
paldepind commented 9 years ago

@ybybzj I see what you mean. In some cases the current approach might increase the amount of error handling boiler plate since it's harder to handle errors in a central place.

If I understand you correctly what you're suggesting is that Flyd automatically does something like this:

var api = require('./api');

function catchAndWrapErr(promise) {
  return promise.catch(function(err) {
    return {isErr: true, err: err};
  });
}

var s1 = flyd.stream(catchAndWrapErr(api.getA()));
var s2 = flyd.stream(catchAndWrapErr(api.getB()));
var s3 = flyd.stream(catchAndWrapErr(api.getC()));

var s = flyd.stream([s1, s2, s3], function(self){
    if (s1().isErr || s2().isErr) || s3().isErr) {
         return {};
    } else {
        return {
             s1: s1(),
             s2: s2(),
             s3: s3()
        };
    }
});

Is that correct?

ybybzj commented 9 years ago

Yeah, something like this. Thanks for your reply! And, users can easily forget to handle errors, maybe try-catch the synchronous code too?

jasonkuhrt commented 9 years ago

When Kefir was developing this feature I suggested that users should deal with Errors as first-class values via an abstraction such as Either. This direction did not get any traction in the discussion, but since one of Flyd's goals is to be a full card-carrying FP citizen, it seems that maybe it will here?

––Edit

Agree:

My current opinion is that Flyd should not concern itself with error handling. I don't see any benefit in doing so.

Disagree:

The idea is that you should never send promises that might reject down a stream.

This is very hostile toward generic programming which will not know enough about errors to handle them at all. You are basically forbidding the ability to build I/O into libraries with this line of thinking.

It seems @ybybzj is intuitively thinking along the same lines. Allow errors to flow through the stream. I am of the opinion that I/O values should be represented as Either values and a library of combinators should be built around composition that transparently operates on only/only-not errors etc.

I will try to find time to write some pseudo code example of what this could look like at the library-layer and at the application-layer.

paldepind commented 9 years ago

one of Flyd's goals is to be a full card-carrying FP citizen

What does full card-carrying mean?

But yes! I had not though of that but using an either type would be a splendid idea. FRP is based on functional programming and an either type is how one typically handles errors with functional programming.

Instead of the previous catchAndWrapErr function you could do this:

function wrapEither(promise) {
  return promise.then(function(result) {
    return Either.Right(result);
  }).catch(function(err) {
    return Either.Left(err);
  });
}

And then wrap promises with that function before you pass them to Flyd.

jasonkuhrt commented 9 years ago

What does full card-carrying mean?

It means being a full-fledged member of an organization or the like. flyd is trying to be a tool that is pure to the functional programming paradigm. I based this opinion on your Readme which states:

[...] aren't functional enough

: )

But yes!

Cool glad you see promise in this direction! Again I'll try to share more ideas later.

ybybzj commented 9 years ago

@jasonkuhrt , looking forward to your pseudo code example. :blush:

kwijibo commented 8 years ago

It would be great if the docs could mention error handling, and at least one of the examples include error handling from various sources (promises, node-backs, etc)

StreetStrider commented 8 years ago

Either is a good idea. There're two considerations here:

  1. Should flyd implement its own Either or there's good stuff on npm?
  2. Should flyd's primitives (like map) be updated to work with Either (e.g map should unwrap Eithers data and just pass error through itself down to catcher), or they will be ok as now?
paldepind commented 8 years ago

Should flyd implement its own Either or there's good stuff on npm?

I don't think Flyd should provide its own Either. There are implementations already and they should work with Flyd just fine.

squiddle commented 8 years ago

could it be useful to provide an error stream like the end stream? So for proper error handling the suggested solution should be using either. The error stream would allow to have error handling for errors in flyd or its modules (which should not handle Either so they must handle all errors).

i like the decision to move the usage of either out of the scope of flyd. Even though it probably means every own function (eg. a map fn) which just needs to pass errors along, would need to be wrapped with a curried fn of Either.chain(func, either) (if the func takes the values in the stream) or flyd.map(Either.chain(func)) (if the func takes the streams).