kjda / ReactFlux

A library implementing React-Flux data flow design pattern + Code Generation
MIT License
66 stars 12 forks source link

Check event listeners are valid before calling #17

Closed aforty closed 9 years ago

aforty commented 9 years ago

I ran across an issue where one of the listeners on an event would be undefined by the time I got to it in the _forEach, this would cause the _SUCCESS event to not finish (after .setState() which triggered the .emit()) and then the _AFTER event to never be called. So I would have 3 valid functions as listeners, but by the time I got to the third one it would be undefined. I can't seems to figure out why or how but it happened, my best guess is that a previous listener invalidated the third somehow. I have React components nested pretty deep, is it possible that unmounting a component caused a child components listener to become undefined?

Either way, putting this sanity check inside the _forEach solved the issue, and there doesn't seem to be anything else wrong with my application. Just seems like good practice.

kjda commented 9 years ago

thanks

aforty commented 9 years ago

This bug was actually really tough to track down. I don't know if it's my setup or something else, but the error went completely unreported and for the longest time I had no knowledge of this bug occurring at all. My code in the "_SUCCESS" block would simply never complete (there was a console.log statement at the end) and the "_AFTER" block would never be called. However, no error, nothing indicating that there was a problem. I quickly looked at the code to figure out why it wasn't throwing an error but I couldn't figure it out.

Perhaps a better discussion for another thread, but I wanted to make you aware of this.

kjda commented 9 years ago

thanks.. I will investigate this when I get some free time :)

kjda commented 9 years ago

maybe a console.log message would be helpful if listner is not defined

aforty commented 9 years ago

I don't actually think that's necessary. I'm pretty sure now it was caused by a component being unmounted during that time, so it's fine since that component was now unmounted (and doesn't need to handle any changes).

I have 3 nested components being notified of changes. First the layout page, second the page itself, and third a popup modal component. That modal component was hidden and unmounted by the page component and thus the modal listener became undefined. As I said, completely fine because it no longer required updates. I don't think that needs to log anything.

However I do think that ReactFlux should have thrown an error instead of swallowing it and continuing on silently as if nothing happened. Again, not sure if ReactFlux is to blame (a quick search through your codebase did not yield any catch statements). Perhaps lodash is to blame for swallowing the error that occurred inside the callback in its forEach.

aforty commented 9 years ago

Here's a quick example. The error thrown in _SUCCESS is never displayed in the console, and the _AFTER method is never called. It fails completely silently. That's a problem.

var ExampleStore = ReactFlux.createStore({
  ...
}, [
  [ExampleConstants.EXAMPLE_SUCCESS, function () {
    throw new Error('Something bad happened...');
  }],

  [ExampleConstants.EXAMPLE_AFTER, function () {
    console.log('This method is never called.');
  }]
]);
kjda commented 9 years ago

thanks for the detailed description of the problem. I think it is fixed now.. it had to do with throwing errors inside a promise