Warn when "false" or "true" is the value of a boolean DOM prop #13372
Conversation
Details of bundled changes.Comparing: 725e499...f70a0eb react-dom
Generated by 🚫 dangerJS |
| it('warns on the ambiguous string value "False"', function() { | ||
| let el; | ||
| expect(() => { | ||
| el = ReactTestUtils.renderIntoDocument(<div hidden="False" />); |
There was a problem hiding this comment.
Since title case False doesn’t have special meaning I don’t think it’s worth testing for.
There was a problem hiding this comment.
Happy to take your steer on this, I'd just like to state my case first for completeness: The thing I was trying to make safer was people drawing inferences from one "boolean" HTML attribute to another. Since enumerated attribute values are case-insensitive, spellCheck="False" does behave differently to hidden="False", in exactly the same way as the respective lowercase variants do. I take your point that most people would be using lowercase to begin with, so this is probably a rarer case, but it does theoretically occur.
To clarify, you're referring to dropping the toLowerCase() in the actual check, as well as removing this test, right?
There was a problem hiding this comment.
Yeah. I see your point but I think it's unlikely enough to be written accidentally and we want to make the checks as cheap as possible. Strict comparison is better than extra toLowerCase call.
| propertyInfo.type === BOOLEAN && | ||
| typeof value === 'string' && | ||
| value.toLowerCase() === 'false' | ||
| ) { |
There was a problem hiding this comment.
Let’s make value === 'false' the first condition. Since it’s rare we won’t have to check most other conditions.
|
I think this makes sense overall. Maybe with warning for |
| @@ -444,7 +444,7 @@ describe('ReactDOMInput', () => { | |||
|
|
|||
| it('should take `defaultValue` when changing to uncontrolled input', () => { | |||
| const node = ReactDOM.render( | |||
| <input type="text" value="0" readOnly="true" />, | |||
| <input type="text" value="0" readOnly={true} />, | |||
There was a problem hiding this comment.
This test was broken by adding the warning on "true". I opted to just fix what the warning was complaining about, which doesn't change the semantics of this test AFAICT.
motiz88
changed the title
|
@gaearon Thanks for the review! All feedback addressed. |
| expect(() => { | ||
| el = ReactTestUtils.renderIntoDocument(<div hidden="true" />); | ||
| }).toWarnDev( | ||
| 'Received the string `true` for the boolean attribute `hidden`. ' + |
There was a problem hiding this comment.
Maybe add Although this works, it will not work as expected if you pass the string "false". to the true case.
|
@gaearon Friendly ping 😄 I've amended the warning messages as per your feedback. Anything else you'd like to see here? |
|
Looks great. Thank you. |
|
This is dead code when using a type system. Could this be enabled only in dev mode perhaps? |
|
Nevermind, didn't see the if (__DEV) :) |
|
Thanks for this PR, @motiz88. It surfaced a 1+ year-old bug in a codebase I work on. 👍 |
Hi! 😄
This is my first contribution that actually changes runtime behaviour, and involved some guesswork as to where to make what change to get the desired effect - so do take it with a grain of salt. I'm super willing to learn more about how React is written and fix anything that needs to be fixed in this PR, if the functionality is welcome.
This PR adds a new DEV-mode warning when the value
"false"(case-insensitive) is found in a DOM prop of typeBOOLEAN, recommending the use of the boolean valuefalseinstead.While working on facebook/flow#6727 I came across the different types of boolean DOM props, and specifically the fact that
BOOLEANprops (e.g.hidden) treat the string"false"as equivalent to booleantrue, whereasBOOLEANISH_STRINGprops (e.g.spellCheck) treat it as equivalent to booleanfalse- an inconsistency that follows from the HTML spec, but one that can conceivably trip up developers, hence a warning.I'm not entirely sure what
OVERLOADED_BOOLEANprops should do about"false"- currently the warning doesn't apply to them.EDIT: Per feedback below, this now also warns on
"true", and only warns on case-sensitive (lowercase) matches.