search

joshburgess / redux-most

Most.js based middleware for Redux. Handle async actions with monadic streams & reactive programming.
https://www.npmjs.com/package/redux-most
MIT License
139 stars 14 forks source link

Passing non-functions to combineEpics() results in unhandled promise rejection. #16

Closed vlad-zhukov closed 3 years ago

vlad-zhukov commented 7 years ago

It shouldn't normally happen, and in node 8 unhandled promise rejections are going to terminate the process.

As a fix I propose to ignore non-function values, this will also help a lot with code splitting/hmr and replaceEpic(). Another way is to catch the error in the stream with recoverWith(), but I don't see the right way how to do it here.

joshburgess commented 7 years ago

@Vlad-Zhukov Please see what I did with combineEpics for the upcoming 0.5.0 version (currently sitting in the dev branch).

https://github.com/joshburgess/redux-most/blob/dev/src/combineEpics.js

I just added extra TypeError checks. I currently throw an error immediately if a non-function is passed in. I thought that this would be a good idea, because it would cause a fail fast scenario where people would catch the problem while developing. Are you okay with this?

vlad-zhukov commented 7 years ago

It doesn't fix the bug, it work the same as before except now it throws a little more meaningful errors. The point is I don't want to wrap every combineEpics with try-catch or write custom type checking logic just to make sure my server won't crash due to a small stupid bug.

Edit:isArrayLike will match strings as well. Why not stick to arrays only?

joshburgess commented 7 years ago

@Vlad-Zhukov I switched to the native Array.isArray and pushed up the change, but I also added another error check that makes sure the return value of each epic call is a stream. This is something that redux-observable does, and I thought it was a good idea.

Why would you ever pass in something that isn't a function? I'm not sure ignoring non-functions is the right thing to do, because it should never happen in the first place. This is an error most people would want to catch during development and would never make it to a production environment.

this will also help a lot with code splitting/hmr and replaceEpic().

Can you elaborate on the above? How will it help in relation to those things?

Another way is to catch the error in the stream with recoverWith(), but I don't see the right way how to do it here.

Yes, this is the proper way to handle errors (for example, network errors during AJAX requests) in your Epics. The idea is to capture the error and attach it to an outgoing action to be handled elsewhere. I should probably add documentation on this. I'm not sure this error handling is related to combineEpics though. I believe we want to catch errors related to comebineEpics immediately, because they will happen as the app first loads, and we want to make sure the developer(s) are aware of a problem in the way they are using comebineEpics.

vlad-zhukov commented 7 years ago

With code splitting and hmr you have to keep track of what epics (and reducers too) are in use right now, and depending on the complexity of that logic sparse arrays like [function, undefined, function] might occur. But I agree it shouldn't happen, so lets leave this part.

My point is unhandled promise rejections should never happen as well. Among other reasons they don't really help in development (or if error happened in production) because they are handled differently in different engines/platforms. For example: