Skip to content

Should throw error when null or falsy values are passed as event listeners #1255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
amasad opened this issue Mar 13, 2014 · 6 comments
Closed

Comments

@amasad
Copy link
Contributor

amasad commented Mar 13, 2014

Null values for event listeners fail silently. http://jsfiddle.net/2BCLQ/1/
Falsy values for event listeners fail silently http://jsfiddle.net/2BCLQ/2/

amasad added a commit to amasad/react that referenced this issue Mar 13, 2014
@sophiebits
Copy link
Collaborator

I'm afraid this behavior is intentional so that you can write things like onChange={isEnabled && this.handleChange}, and I don't think we're going to change this.

@amasad
Copy link
Contributor Author

amasad commented Mar 13, 2014

this explains the falsey part but not why failing silently for null value. Passing null values as event listeners should at least be warned.

@sophiebits
Copy link
Collaborator

Similarly, you could write onChange={isEnabled ? this.handleChange : null} or something along those lines. We've felt like that is the most helpful behavior; it's convenient to have falsey handlers be ignored, just like null and false don't render anything when included in an element's children.

@amasad
Copy link
Contributor Author

amasad commented Mar 13, 2014

Right. Have you thought about standardizing on boolean false instead? onChange={isEnabled ? this.handleChange : false} onChange={isEnabled && this.handleChange} works fine with that.

Maybe not worth breaking change, but we tripped over this a couple of times where someone changes a method name or a typo etc.

@sophiebits
Copy link
Collaborator

I personally find it much weirder that false is ignored; it doesn't surprise me that null is. With props, we consistently assume that undefined is equivalent to not specifying the prop at all (like when merging in the result of getDefaultProps) so it would definitely be a change to treat undefined as invalid here.

@akre54
Copy link
Contributor

akre54 commented Mar 10, 2015

I understand the reasoning behind keeping the events and props interface similar but this has bit me a few times on stupid typos and refactorings. React is usually very good about failing early and loudly in dev for other gotchas, and this seems like an odd oversight.

IMO null and false are both fine sentinels for "this space intentionally left blank" but undefined is Javascript's idiotic way of handling both "not here right now" and "was never here". It'd be nice to have this addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants