Closed shaoshing closed 8 years ago
I definitely like highlighting slow actions. I also like using a console group for errors, but I'm a little worried that errors are a little less visible as a result of this change. The nice thing about the current behavior is that it's very hard to miss a console error. Any thoughts from anyone else?
I agree with Ian, can we use log.error
instead?
I manually trigger some errors as a comparison with the action message and the change seems to have small impact on the current behavior, and the error messages are still fairly easy to find.
(groups are collapsed by default. )
@iwehrman Shaoshing's change introduces stack traces for all "Executing action..." logs, that are custom to see.
The Error: Action Trace
makes it a bit confusing, but that's only to get the stack trace. There is no actual error.
So I am ok with this change, actual errors are still in red and blaring.
Hmm, do we actually want traces for all executed actions? Aren't they all going to look basically like the one in the original screenshot?
I thought having bunch of bluebird/fluxcontroller.js stack traces would be useless, too... But if you start an action through a React element / listener, they actually show those in their traces. And since they're all neatly grouped and not shown until you see them... I don't have a qualm with it.
@iwehrman this would be helpful when you need to know why an action is called. For example, I saw a bunch of panel.updatePanelSizes
is called at startup. I can expand each action to trace the root, without manually output the trace.
I agree that this is not difficulty to add manually when needed, so I am open to removing this custom log, if you guys do not think it is worth of doing.
OK, we're making progress, but here's my next gripe: it's confusing that each of these traces starts with Error
. (That's why I griped originally about errors not being red... I thought the one open stack trace was an actual error. ) I think if we can fix that, and also make sure that none of this is happening in debug mode, then I'm OK with it. For the latter, that means not doing new Error().stack
if __PG_DEBUG__
is true, because I suspect that has a somewhat significant overhead.
@volfied @iwehrman the action stack trace can be misleading, and it is easy to log the trace manually when needed, so I think it might be not worth of adding to our code base. Now the only change this PR made is highlighting slow actions. Thanks for your suggestions. Please review again.
Sorry to have reviewed this half to death, but what's left is a nice improvement!
This PR adds the following improvement to the debug messages:
Includes stack trace in a console group when messageExecuting Action ...
, which make it easier to know where triggers the action.