Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2532,6 +2532,45 @@ describe('ReactDOMComponent', () => {

expect(el.getAttribute('hidden')).toBe('');
});

it('warns on the ambiguous string value "" when it means false', function() {
let el;
expect(() => {
el = ReactTestUtils.renderIntoDocument(<div hidden="" />);
}).toWarnDev(
'Received the string "" for the boolean attribute `hidden`. ' +
'This value can mean `true` or `false`, depending on the attribute. ' +
'Did you mean hidden={false}?',
);

expect(el.hasAttribute('hidden')).toBe(false);
});

it('warns on the ambiguous string value "" when it means true', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arguably this test belongs under String boolean attributes above, rather than here under Boolean attributes (a test category I introduced in #13372), but then again it's clearly a sibling test to the other two that do belong here. 🤷‍♂️ Happy to restructure this based on feedback.

let el;
expect(() => {
el = ReactTestUtils.renderIntoDocument(<div spellCheck="" />);
}).toWarnDev(
'Received the string "" for the boolean attribute `spellCheck`. ' +
'This value can mean `true` or `false`, depending on the attribute. ' +
'Did you mean spellCheck={true}?',
);

expect(el.getAttribute('spellCheck')).toBe('');
});

it('warns on the ambiguous string value "" in an overloaded boolean prop', function() {
let el;
expect(() => {
el = ReactTestUtils.renderIntoDocument(<input capture="" />);
}).toWarnDev(
'Received the string "" for the boolean attribute `capture`. ' +
'This value can mean `true` or `false`, depending on the attribute. ' +
'Did you mean capture={true}?',
);

expect(el.getAttribute('capture')).toBe('');
});
});

describe('Hyphenated SVG elements', function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ describe('ReactDOMServerIntegration', () => {
// that the boolean property is present. however, it is how the current code
// behaves, so the test is included here.
itRenders('boolean prop with "" value', async render => {
const e = await render(<div hidden="" />);
const e = await render(<div hidden="" />, 1);
expect(e.getAttribute('hidden')).toBe(null);
});

Expand Down
28 changes: 27 additions & 1 deletion packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import warning from 'shared/warning';
import {
ATTRIBUTE_NAME_CHAR,
BOOLEAN,
BOOLEANISH_STRING,
OVERLOADED_BOOLEAN,
RESERVED,
shouldRemoveAttributeWithWarning,
getPropertyInfo,
Expand Down Expand Up @@ -227,7 +229,7 @@ if (__DEV__) {
return false;
}

// Warn when passing the strings 'false' or 'true' into a boolean prop
// Warn about passing the strings 'false' or 'true' into a boolean prop
if (
(value === 'false' || value === 'true') &&
propertyInfo !== null &&
Expand All @@ -250,6 +252,30 @@ if (__DEV__) {
return true;
}

// Warn about passing an empty string into any kind of boolean prop, except
// 'value' which is modeled as a "booleanish string"
if (
value === '' &&
name !== 'value' &&
propertyInfo !== null &&
(propertyInfo.type === BOOLEAN ||
propertyInfo.type === BOOLEANISH_STRING ||
propertyInfo.type === OVERLOADED_BOOLEAN)
) {
const isBoolean = propertyInfo.type === BOOLEAN;
warning(
false,
'Received the string "" for the boolean attribute `%s`. ' +
'This value can mean `true` or `false`, depending on the attribute. ' +
'Did you mean %s={%s}?',
name,
name,
isBoolean ? 'false' : 'true',
);
warnedProperties[name] = true;
return true;
}

return true;
};
}
Expand Down