kenwheeler / mcfly

Flux architecture made easy
BSD 3-Clause "New" or "Revised" License
762 stars 46 forks source link

Hidden errors #29

Closed andystudios closed 9 years ago

andystudios commented 9 years ago

I've been using mcfly for a month now and I love it. My project now is getting bigger and bigger and I found out a very annoying issue.

Many times errors in the onChange, or render Methods have any output in the console. The only way to make then visible is to add try catch blocks inside each method.

Do you have any kind of clue why this is happening and how could I solve it? I'm using react 0.12.2 and mcfly 0.0.8.

Thanks in advance!!

kenwheeler commented 9 years ago

Can I see a code sample to help diagnose what's going on?

andystudios commented 9 years ago

I think I found something.

I placed something like this in one of my stores:

case "AfterAjaxCall":

throw "stupidError";
MyStore.emitChange();

The emitChange then was never executed, but I got no errors in the console. Also I place the "throw" in the render method of the component listening to that EmitChange and also I couldn't see anything in the console and the code got blocked.

I found out that if I place A try catch in the Action.js file, in the mcFly lib, then I can see both errors in the console:

  Action.prototype.dispatch=function() {"use strict";
    return Promise.resolve(this.callback.apply(this, arguments))
      .then(function(payload){
        return new Promise(function(resolve, reject){

try{

          if (!payload) return reject();
          if (!payload.actionType) return reThrow(reject,
            "Payload object requires an actionType property"
          );
          Dispatcher.dispatch(payload)
          resolve();

}catch(e){ console.error(e); };

        });
      });
  };

I don't know much about promises, seems this problem is related with this blog post: http://bahmutov.calepin.co/why-promises-need-to-be-done.html

This error swallowing is a little nightmare :P

eglinetwork commented 9 years ago

The pull request Fixes an issue where errors in stores would be suppressed https://github.com/kenwheeler/mcfly/pull/24 maybe adresses the same problem

ferrannp commented 9 years ago

+1. I am facing the same problem, I can write this in my store:

console.log(whatEverThatIsUndefined);
Store.emitChange();

emitChange is not called but I don't see any error. Also happening in render function.

sjmarshy commented 9 years ago

+1

tomatau commented 9 years ago

It's a common problem that promises need catching and tend to silently swallow errors - It's generally better to just always handle your promises.

kenwheeler commented 9 years ago

So what do you guys think? Should the responsibility to handle errors within promises be the developers responsibility? Anyone have any ideas regarding alternatives?

sjmarshy commented 9 years ago

I think in the case of a library/framework like this, if it's the app developers code running, and the app developers code throws an error it should be their error to handle. It's happened a couple of times I've had errors hidden and only realised later on

ferrannp commented 9 years ago

I think I am missing something, if I do something like this:

onChange: function () {
  console.log(undefinedVariable);
  this.setState(Store.getState());
}

My component is not getting updated and I don't see any errors. Is this because McFly promise? I think it is difficult to see that.... By the way, how would we handle the error ?

By the way, is solution from https://github.com/kenwheeler/mcfly/pull/24 not correct? Doing like this:

        try {
          Dispatcher.dispatch(payload);
        } catch (error) {
          reThrow(reject, error);
        }

Then I can see the error:

Uncaught ReferenceError: undefinedVariable is not defined
tomatau commented 9 years ago

I agree with sjmarshy and the pull request looks nice to me too.

Althought, I don't get how Store and render error's are being suppressed by the actionFactory's dispatch promises..?

kenwheeler commented 9 years ago

ok @tomatau I'll review it for inclusion either tonight or tomorrow. Trying to put a dent in some issues on my "slick" repo today.

kenwheeler commented 9 years ago

Merged