Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Boolean attributes no longer coerced to string #9779

Closed
wants to merge 3 commits into from
Closed

Boolean attributes no longer coerced to string #9779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3d16ba
Custom boolean attributes are not coerced to strings
May 25, 2017
cf9ee44
updating boolean attribute docs
May 25, 2017
c1fb100
run fiber
May 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/docs/jsx-in-depth.md
View file
Open in desktop
Expand Up @@ -218,9 +218,9 @@ When you pass a string literal, its value is HTML-unescaped. So these two JSX ex

This behavior is usually not relevant. It's only mentioned here for completeness.

### Props Default to "True"
### Props Default to Empty String

If you pass no value for a prop, it defaults to `true`. These two JSX expressions are equivalent:
If you pass no value for a prop, it defaults to ''. These two JSX expressions are equivalent:

```js
<MyTextBox autocomplete />
Expand Down
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Expand Up @@ -800,6 +800,7 @@ src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
* should set values as namespace attributes if necessary
* should set values as boolean properties
* should convert attribute values to string first
* should set values as boolean attributes
* should not remove empty attributes for special properties
* should remove for falsey boolean properties
* should remove when setting custom attr to null
Expand Down
4 changes: 4 additions & 0 deletions src/renderers/dom/shared/DOMPropertyOperations.js
View file
Open in desktop
Expand Up @@ -188,6 +188,10 @@ var DOMPropertyOperations = {
}
if (value == null) {
node.removeAttribute(name);
} else if (value === true) {
node.setAttribute(name, '');
} else if (value === false) {
node.removeAttribute(name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change worries me. This would be breaking behavior since we currently just stringify false and this would potentially change runtime behavior. I think we should leave this specific part as-is, where null is used to identify attributes that should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. spellcheck is not a boolean attribute and expects the value 'true' or 'false', if anyone relies on boolean being stringified today it will break for them (I assume? or is that another codepath). Also note that neither 'true' nor 'false' is the default value for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other enumerated attributes like that which use stringified booleans? I would hate to special case them, but I also think it's reasonable to ask users to provide the expected stringified values themselves, e.g., spellcheck="true". We could have dev-only warnings for that maybe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, using "true" and "false" but not making it a boolean attribute seems like a really confusing decision 🤔

Copy link
Contributor

@syranide syranide May 29, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9806 seems relevant to this as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if there is any cross over with #9806, but I'm curious, if we move forward with this, how value should be treated.

We have a special mutation method for value that might need to be synced up with this new logic, but as far as I see it, value={true} should stringify to "true", which is what would happen if you mutated the input directly.

} else {
node.setAttribute(name, '' + value);
}
Expand Down
7 changes: 7 additions & 0 deletions src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
View file
Open in desktop
Expand Up @@ -203,6 +203,13 @@ describe('DOMPropertyOperations', () => {
expect(stubNode.getAttribute('role')).toBe('<html>');
});

it('should set values as boolean attributes', () => {
DOMPropertyOperations.setValueForProperty(stubNode, 'data-foo', true);
expect(stubNode.getAttribute('data-foo')).toBe('');
DOMPropertyOperations.setValueForProperty(stubNode, 'data-foo', false);
expect(stubNode.getAttribute('data-foo')).toBe(null);
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true? No matter how I assign data attributes, they always appear, regardless of boolean values:

https://codepen.io/nhunzaker/pen/YVoYMV?editors=1010

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct; DOMStringMap (the backing type for .dataset) always only holds string values.

it('should not remove empty attributes for special properties', () => {
stubNode = document.createElement('input');
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);
Expand Down