move combine reducers sanity check #717 #761
Conversation
| if (sanityError) { | ||
| console.error(sanityError.message); | ||
| } | ||
|
|
There was a problem hiding this comment.
Not sure that this console.error is necessary, the error is thrown very shortly afterwards. And if it is useful, it has just occurred to me that it should probably log error.stack if available.
There was a problem hiding this comment.
console.logs/errors are a no-no in the library because they are normally uncatchable and noisy (and console sometimes doesn't exist). A thrown Error is best because that follows normal JS patterns.
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't think console.error is inherently bad if it is a sign of an error. React also uses console.error for propTypes warnings because they're not really warnings: if you have them, you need to fix them. The nice thing about console.error is that it prints the stack trace, unlike its friends.
There was a problem hiding this comment.
Doesn't it disable logging in the production build? Maybe the same thing should happen here. A log isn't bad for devs, but shouldn't make its way to production.
There was a problem hiding this comment.
Yes, we previously console.error'd only in development, too.
I think we should hide this console.error behind the dev flag as well.
Of course, if it's not helpful, it's better to remove it. I'm not sure whether the confusion from error printed twice is bigger than the help from a proper stack trace. Thoughts?
There was a problem hiding this comment.
I've played with this a bit more using devtools and I don't think the console.error is helpful. Throwing sanityError logs the stack trace from where the error was created in combineReducers, so the two stack traces in the log are actually the same.
There was a problem hiding this comment.
Oh, okay. Then let's remove it! Thanks for checking.
ellbee
force-pushed
the
move_combineReducers_sanity_check
branch
from
0354a42 to
df287f3
Compare
|
Updated to create but not log the error during |
ellbee
force-pushed
the
move_combineReducers_sanity_check
branch
from
df287f3 to
7bc27de
Compare
|
Added production check around sanity check (#771) |
|
It's only OK to check for production env for errors we don't throw. Both environments should throw in the same conditions so error stack traces from production are reproducible in development. Please see #771 (comment). |
|
Got it. I'll revert back to how it was previously. |
|
Sure, thanks! |
ellbee
force-pushed
the
move_combineReducers_sanity_check
branch
from
7bc27de to
c2bf13c
Compare
move combine reducers sanity check #717
Creates and logs sanity check error during
combineReducers, but only throws on first invocation. Addresses #717.