Skip to content

Commit

Permalink
Fixed invalid prop types error message to be more specific (facebook#…
Browse files Browse the repository at this point in the history
…11308)

* Modified tests and corrected error message. #3

* Fixed syntax issues. #3

* Modified test. #3

* Prettified. #3

* Changed warning message to handle true and false boolean values. #3

* Changed test to contain undefined instead of value. #3

* Simplified branch structure. #3

* Refactored branching logic. #3

* Refactored falsy warning message and tests. #3

* Changed condition to attribute name. #3

* Refactored falsy and truthy warning messages with tests updated. #3

* Added missing character. #3

* Fixed warning message. #3

* Cleared extra whitespace. #3

* Refactored warning messages to be clear. #3

* Prettified. #3

* Grammar fix

* Tweak unrelated warning

The message didn't make sense because it appears for *any* attributes, not just numeric ones.

* Tweak the message for more clarity

* Add a special message for false event handlers

* Add missing whitespace

* Revert size changes
  • Loading branch information
NicBonetto authored and gaearon committed Oct 31, 2017
1 parent 3f1f3dc commit 544d5c7
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 27 deletions.
7 changes: 4 additions & 3 deletions packages/react-dom/src/__tests__/ReactDOMAttribute-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@ describe('ReactDOM unknown attribute', () => {
expectDev(
normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]),
).toMatch(
'Warning: Received `true` for non-boolean attribute `unknown`. ' +
'If this is expected, cast the value to a string.\n' +
'Received `true` for a non-boolean attribute `unknown`.\n\n' +
'If you want to write it to the DOM, pass a string instead: ' +
'unknown="true" or unknown={value.toString()}.\n' +
' in div (at **)',
);
expectDev(console.error.calls.count()).toBe(1);
Expand Down Expand Up @@ -87,7 +88,7 @@ describe('ReactDOM unknown attribute', () => {
expectDev(
normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]),
).toMatch(
'Warning: Received NaN for numeric attribute `unknown`. ' +
'Warning: Received NaN for the `unknown` attribute. ' +
'If this is expected, cast the value to a string.\n' +
' in div (at **)',
);
Expand Down
22 changes: 16 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ describe('ReactDOMComponent', () => {

expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Received a `function` for string attribute `is`. If this is expected, cast ' +
'Received a `function` for a string attribute `is`. If this is expected, cast ' +
'the value to a string.',
);
});
Expand Down Expand Up @@ -2042,7 +2042,9 @@ describe('ReactDOMComponent', () => {
expect(el.hasAttribute('whatever')).toBe(false);

expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Received `true` for non-boolean attribute `whatever`',
'Received `true` for a non-boolean attribute `whatever`.\n\n' +
'If you want to write it to the DOM, pass a string instead: ' +
'whatever="true" or whatever={value.toString()}.',
);
});

Expand All @@ -2055,7 +2057,9 @@ describe('ReactDOMComponent', () => {
expect(el.hasAttribute('whatever')).toBe(false);

expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Received `true` for non-boolean attribute `whatever`',
'Received `true` for a non-boolean attribute `whatever`.\n\n' +
'If you want to write it to the DOM, pass a string instead: ' +
'whatever="true" or whatever={value.toString()}.',
);
});

Expand Down Expand Up @@ -2128,7 +2132,7 @@ describe('ReactDOMComponent', () => {
expect(el.getAttribute('whatever')).toBe('NaN');

expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Received NaN for numeric attribute `whatever`. If this is ' +
'Warning: Received NaN for the `whatever` attribute. If this is ' +
'expected, cast the value to a string.\n in div',
);
});
Expand Down Expand Up @@ -2230,7 +2234,9 @@ describe('ReactDOMComponent', () => {
expect(el.hasAttribute('whatever')).toBe(false);

expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Received `true` for non-boolean attribute `whatever`.',
'Received `true` for a non-boolean attribute `whatever`.\n\n' +
'If you want to write it to the DOM, pass a string instead: ' +
'whatever="true" or whatever={value.toString()}.',
);
});

Expand Down Expand Up @@ -2283,7 +2289,11 @@ describe('ReactDOMComponent', () => {

expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Received `false` for non-boolean attribute `whatever`.',
'Received `false` for a non-boolean attribute `whatever`.\n\n' +
'If you want to write it to the DOM, pass a string instead: ' +
'whatever="false" or whatever={value.toString()}.\n\n' +
'If you used to conditionally omit it with whatever={condition && value}, ' +
'pass whatever={condition ? value : undefined} instead.',
);
});
});
Expand Down
19 changes: 19 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -945,6 +945,25 @@ describe('ReactDOMFiber', () => {
);
});

it('should warn with a special message for `false` event listeners', () => {
spyOn(console, 'error');
class Example extends React.Component {
render() {
return <div onClick={false} />;
}
}
ReactDOM.render(<Example />, container);
expectDev(console.error.calls.count()).toBe(1);
expectDev(
normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]),
).toContain(
'Expected `onClick` listener to be a function, instead got `false`.\n\n' +
'If you used to conditionally omit it with onClick={condition && value}, ' +
'pass onClick={condition ? value : undefined} instead.\n',
' in div (at **)\n' + ' in Example (at **)',
);
});

it('should not update event handlers until commit', () => {
let ops = [];
const handlerA = () => ops.push('A');
Expand Down
27 changes: 20 additions & 7 deletions packages/react-dom/src/client/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,26 @@ if (__DEV__) {
};

var warnForInvalidEventListener = function(registrationName, listener) {
warning(
false,
'Expected `%s` listener to be a function, instead got a value of `%s` type.%s',
registrationName,
typeof listener,
getCurrentFiberStackAddendum(),
);
if (listener === false) {
warning(
false,
'Expected `%s` listener to be a function, instead got `false`.\n\n' +
'If you used to conditionally omit it with %s={condition && value}, ' +
'pass %s={condition ? value : undefined} instead.%s',
registrationName,
registrationName,
registrationName,
getCurrentFiberStackAddendum(),
);
} else {
warning(
false,
'Expected `%s` listener to be a function, instead got a value of `%s` type.%s',
registrationName,
typeof listener,
getCurrentFiberStackAddendum(),
);
}
};

// Parse the HTML and read it back to normalize the HTML string so that it
Expand Down
48 changes: 37 additions & 11 deletions packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ if (__DEV__) {
) {
warning(
false,
'Received a `%s` for string attribute `is`. If this is expected, cast ' +
'Received a `%s` for a string attribute `is`. If this is expected, cast ' +
'the value to a string.%s',
typeof value,
getStackAddendum(),
Expand All @@ -136,7 +136,7 @@ if (__DEV__) {
if (typeof value === 'number' && isNaN(value)) {
warning(
false,
'Received NaN for numeric attribute `%s`. If this is expected, cast ' +
'Received NaN for the `%s` attribute. If this is expected, cast ' +
'the value to a string.%s',
name,
getStackAddendum(),
Expand Down Expand Up @@ -179,15 +179,41 @@ if (__DEV__) {
return true;
}

if (typeof value === 'boolean') {
warning(
DOMProperty.shouldAttributeAcceptBooleanValue(name),
'Received `%s` for non-boolean attribute `%s`. If this is expected, cast ' +
'the value to a string.%s',
value,
name,
getStackAddendum(),
);
if (
typeof value === 'boolean' &&
!DOMProperty.shouldAttributeAcceptBooleanValue(name)
) {
if (value) {
warning(
false,
'Received `%s` for a non-boolean attribute `%s`.\n\n' +
'If you want to write it to the DOM, pass a string instead: ' +
'%s="%s" or %s={value.toString()}.%s',
value,
name,
name,
value,
name,
getStackAddendum(),
);
} else {
warning(
false,
'Received `%s` for a non-boolean attribute `%s`.\n\n' +
'If you want to write it to the DOM, pass a string instead: ' +
'%s="%s" or %s={value.toString()}.\n\n' +
'If you used to conditionally omit it with %s={condition && value}, ' +
'pass %s={condition ? value : undefined} instead.%s',
value,
name,
name,
value,
name,
name,
name,
getStackAddendum(),
);
}
warnedProperties[name] = true;
return true;
}
Expand Down

0 comments on commit 544d5c7

Please sign in to comment.