Warn against custom non-lowercase attributes#10699
Conversation
| 'React does not recognize the `helloWorld` prop on a DOM element. ' + | ||
| 'If you intentionally wanted it to appear in the DOM as a custom ' + | ||
| 'attribute, spell it as the lowercase `helloworld` instead. ' + | ||
| 'If you passed it accidentally from a parent component, remove ' + |
There was a problem hiding this comment.
I think it’s important to mention this since people will often get this warning due to spreading.
There was a problem hiding this comment.
It seems like practically this is going to make the unknown DOM warning permanent (modulo some cases). Granted it's probably not what the user intends it seems a bit heavy handed.
There was a problem hiding this comment.
or perhaps maybe just that it's going to give the impression that everything is fine when they aren't getting this warning, while foo="[Object object]" still gets passed
There was a problem hiding this comment.
The react prop name can still have casing, as long as it's passed as lowercase to the HTML element. This doesn't allow spreading of attributes, but you can do it at least.
I think it's important to communicate what this is protecting against. I don't know that this should live in the warning, but I do agree that a developer not familiar with the problem-space could see this as an arbitrary restriction.
We could turn it into a "React's got your back" kind of thing if we effectively communicate it.
There was a problem hiding this comment.
What about custom elements (or does that not hit here)? Chance of a false positive? If we are trying to get out of folks way about passing what they want to the DOM this feels a bit arbitrary as you say.
There was a problem hiding this comment.
sigh didn't see the original issue, sorry for the noise :P
| const e = await render(<div data-fooBar="true" />); | ||
| expect(e.getAttribute('data-fooBar')).toBe('true'); | ||
| const e = await render(<div data-fooBar="true" />, 1); | ||
| expect(e.getAttribute('data-foobar')).toBe('true'); |
There was a problem hiding this comment.
Changing these calls wasn't essential but I figured I'd emphasize they are normalized by the browser anyway.
| const e = await render(<div data-fooBar={true} />); | ||
| expect(e.getAttribute('data-fooBar')).toBe('true'); | ||
| const e = await render(<div data-foobar={true} />); | ||
| expect(e.getAttribute('data-foobar')).toBe('true'); |
There was a problem hiding this comment.
These tests were not specifically about casing so I fixed them.
| var rARIA = new RegExp('^(aria)-[' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$'); | ||
| var rARIACamel = new RegExp( | ||
| '^(aria)[A-Z][' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$', | ||
| ); |
There was a problem hiding this comment.
Copy pasta from ARIA hook. Catches more cases which ensures we don't fire two warnings instead of one.
nhunzaker
left a comment
nhunzaker
left a comment
There was a problem hiding this comment.
Looks good, but I think this warning is going to require some community education. We need to broadly communicate what pitfalls we're protecting against.
| | Test Case | Flags | Result | | ||
| | --- | --- | --- | | ||
| | `as=(string)`| (changed)| `"a string"` | | ||
| | `as=(string)`| (initial)| `<empty string>` | |
There was a problem hiding this comment.
This is an unrelated change, and seems like Chrome version difference. Setting <link as="whatever"> doesn't return "whatever" from link.as property anymore. But it works with valid values like "audio".
|
I agree but it’s also a bit hard to guess in advance what kind of things people would be using this for. So I’d wait until 16 and then look at these warnings again and maybe unify them more. |
| | `selectedIndex=(-1)`| (initial)| `<number: -1>` | | ||
| | `selectedIndex=(0)`| (initial)| `<number: -1>` | | ||
| | `selectedIndex=(integer)`| (initial)| `<number: -1>` | | ||
| | `selectedIndex=(string)`| (initial, warning)| `<number: -1>` | |
There was a problem hiding this comment.
I love that we finally got a nice warning on these for free.
There was a problem hiding this comment.
Although I guess this will just make people try selectedindex instead. 😕
5 participants
Fixes #10590.