-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
Fixed invalid prop types error message to be more specific #11308
Changes from 7 commits
2653595
8de9a20
4ce5b49
3d4724e
449b98d
bb7978c
05ecf0a
e18a235
9fc2b7f
ede38f3
ae6fcbf
aada64c
de26d9d
c7ee43d
2c19ea3
37207a0
12db856
69cbdfc
c841d41
da20477
5b994bf
425c00b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2042,7 +2042,8 @@ 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 non-boolean attribute `whatever`. If this is expected, cast ' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would look nicer if |
||
'the value to a string.', | ||
); | ||
}); | ||
|
||
|
@@ -2055,7 +2056,8 @@ 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 non-boolean attribute `whatever`. If this is expected, cast ' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
'the value to a string.', | ||
); | ||
}); | ||
|
||
|
@@ -2230,7 +2232,8 @@ 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 non-boolean attribute `whatever`. If this is expected, cast ' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
'the value to a string.', | ||
); | ||
}); | ||
|
||
|
@@ -2283,7 +2286,9 @@ 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`.', | ||
'If you mean to conditionally pass an attribute, use a ternary ' + | ||
'expression: `false`={condition ? value : undefined} instead of ' + | ||
'{condition && value}.', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does Please fix this to show the attribute name. |
||
); | ||
}); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,14 +181,25 @@ if (__DEV__) { | |
} | ||
|
||
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 (value === true) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: we already know it's a boolean so can just write |
||
warning( | ||
DOMProperty.shouldAttributeAcceptBooleanValue(name), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes :) Just so I have a correct understanding, the refactor will look like:
Is this the branching logic you prefer? I do not understand the !false condition, so if I am incorrect please explain what should take it's place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if (typeof value === 'boolean' && !DOMProperty.shouldAttributeAcceptBooleanValue(name)) {
if (value === true) {
warning(false,
...
);
} else {
warning(false,
...
);
}
...
} |
||
'Received `%s` for non-boolean attribute `%s`. If this is expected, cast ' + | ||
'the value to a string.%s', | ||
value, | ||
name, | ||
getStackAddendum(), | ||
); | ||
} else { | ||
warning( | ||
DOMProperty.shouldAttributeAcceptBooleanValue(name), | ||
'If you mean to conditionally pass an attribute, use a ternary ' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this also start with |
||
'expression: `%s`={condition ? value : undefined} instead of ' + | ||
'{condition && value}.%s', | ||
value, | ||
getStackAddendum(), | ||
); | ||
} | ||
warnedProperties[name] = true; | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this removed? |
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unnecessary, right? It just reformats the message in a slightly less readable way.