Warn if shouldComponentUpdate returns anything other than true/false

[danielschonfeld]

This commit should handle the issue raised in #1391

[syranide]

Way too strict in my opinion, simply warning on undefined solves @spicyj's case without having to booleanize every value (which could likely lead to just ignoring the warning because it's far too common).

[danielschonfeld]

I'm open to feedback, I can change the logic to only check for undefined. I was just under the assumption that shouldComponentUpdate should really only return a true/false answer and hence answer the question.

[syranide]

That makes logical, but not practical, sense to me. Most things you pass around as booleans in JavaScript are not actually booleans (|| and && does not cast to booleans).

[petehunt]

Yeah I think only checking for undefined makes the most sense here.

[zpao]

This moves code that was try/catch outside so we might end up in some failure cases we didn't previously experience. That said, it's probably ok.

But you can drop the surrounding parens here.

[syranide]

@zpao Considering it calls this.shouldComponentUpdate(nextProps, nextState, nextContext)); it's probably not a good idea to leave it outside.

[zpao]

Meh, we don't try/catch any other user-specified lifecycle functions so at least we'll be consistent.

[sophiebits]

That's because we don't have anything to put in a finally block. Here _compositeLifeCycleState will be left at RECEIVING_STATE if shouldComponentUpdate throws which is bad.

[danielschonfeld]

so are we good leaving it outside the try/catch? I'll remove the parens.

[sophiebits]

I'd like it to be in the try/finally.

[danielschonfeld]

will do

[sophiebits]

Thanks. :)

[danielschonfeld]

let me know if you guys want to take it in a different direction, otherwise here's everything changed as discussed.
cc @zpao @syranide @spicyj

[sophiebits]

can you add a if (__DEV__ && here?

[danielschonfeld]

I thought it was referenced before that we want to remove the DEV question...?

[sophiebits]

All warnings are shown only in DEV. The warning() helper checks __DEV__ for you but when using console.warn directly you need to check it by hand.

Side note: someone else will probably come along and ask you to change this to typeof shouldUpdate === "undefined" because in theory you could redefine undefined.

[sophiebits]

Looks pretty good, a few inline comments -- thanks for working on this. :)

[danielschonfeld]

Alright, let's try it again :) @spicyj - no problem! I think the React style of thinking is the future way front end should be done, I'm excited to be working on this.

[sophiebits]

Ah, I didn't mean to get rid of the mention of undefined in the message -- sorry! What do you think of:

(this.constructor.displayName || 'ReactCompositeComponent') + '.shouldComponentUpdate(): Returned undefined instead of a ' + 'boolean value. Make sure to return true or false.' 

(And FB style guide says single quotes.)

Otherwise I think this looks great now. Do you mind squashing your commits into one (using git rebase -i)? You'll need to force-push to your branch but GitHub will update the PR automatically. After that I'll let @zpao double-check and merge.

[danielschonfeld]

Sounds good to me and no problem on the rebase... I wasn't sure how you guys work this and if you guys wanted to do the rebase a moment before merging. Will do both

[sophiebits]

Either way; usually I just amend and push unless the incremental changeset is particularly useful to look at on its own.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[syranide]

AFAIK, you can remove if (__DEV__) {, console.warn is automatically stripped in production builds, meaning it'll get removed anyway. @spicyj @petehunt invariant or console.warn? invariant kind of makes sense to me (but I don't really care).

I also think we settled on using value === undefined for testing for undefined, if undefined is overwritten all hope is lost anyway.

[sophiebits]

No, console.warn isn't stripped in __DEV__ currently (see vendor/constants.js). Let's leave the check dev-only (and we don't have dev-only invariants because we want the dev and prod behavior to match exactly).

[syranide]

@spicyj Hmm? I'm pretty sure console.warn is stripped in production-builds, which means the __DEV__ check should be superflous or did I misunderstand you?

Also my point about invariant, I'm pondering if we want to aggressively forbid it (seems like a bad practice to rely on not explicitly returning). Especially as it's not intuitive whether no return means true or false.

[zpao]

No, leave it as is. console.warn isn't stripped. We'll do it like this for now, and maybe be more aggressive if we see it become a larger issue.

[zpao]

Thanks and sorry for the delay!