Enable warnings for the defaultValue prop #13360
Conversation
| @@ -214,12 +214,6 @@ if (__DEV__) { | |||
| return true; | |||
| } | |||
|
|
|||
| // Now that we've validated casing, do not validate | |||
| // data types for reserved props | |||
| if (isReserved) { | |||
There was a problem hiding this comment.
I decided to remove this since the shouldRemoveAttributeWithWarning function already duplicates this logic internally.
…tributeWithWarning
| @@ -115,7 +115,13 @@ export function shouldRemoveAttributeWithWarning( | |||
| propertyInfo: PropertyInfo | null, | |||
| isCustomComponentTag: boolean, | |||
| ): boolean { | |||
| if (propertyInfo !== null && propertyInfo.type === RESERVED) { | |||
| // Do not validate data types for reserved props | |||
| // (excluding defaultValue) | |||
There was a problem hiding this comment.
This is a bit odd. Maybe we shouldn’t treat it as reserved?
There was a problem hiding this comment.
I agree, but wouldn't we then be committing unwanted attributes to the DOM?
There was a problem hiding this comment.
I don’t know. You tell me what would happen :-)
There was a problem hiding this comment.
I went ahead and uncommented defaultValue from the list of reserved properties and a few test suites started to fail in a similar manner (for e.g, ReactDOMServerIntegrationForms), confirming what I was expecting.
E.g.
This seems to only affect SSR, client side cases seem to pass, for e.g:
const div = document.createElement('div');
const node = ReactDOM.render(<textarea defaultValue="1" />, div);
expect(node.getAttribute('defaultValue')).toBe(null);
I'm not too certain nor familiar with how strict we want to be about reserved properties (committing them to the DOM is definitely out of the question, I would assume).
Maybe we can keep the attribute reserved as is, but instead add another key to PropertyInfo (e.g. boolean shouldWarnInDev) to enable warnings for custom properties. This way the condition that you mentioned above would become:
if (
propertyInfo !== null &&
propertyInfo.type === RESERVED &&
!propertyInfo.shouldWarnInDev
) {
return false;
}
Also, enabling warnings for other reserved properties would be a breeze this way, IMHO :-)
There was a problem hiding this comment.
I've added a proof of concept commit to illustrate my approach better.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Somewhat related to #11734.
I started off with adding warnings for assigning symbols and functions to the defaultValue prop (ultimately resolving TODOs in
ReactDOMInput-test.js), then quickly realized that there are a few cases that don't currently warn, so I decided to go ahead and cover those as well. For i.e, assigning a boolean to defaultValue currently does not warn, but could, in order to prevent potential pitfalls or misconceptions.