Add new dom-elements-no-danger-with-children rule (Fixes #710) #719
Conversation
| invalid: [ | ||
| { | ||
| code: [ | ||
| '<div dangerouslySetInnerHTML={{ __html: "HTML" }}>', |
There was a problem hiding this comment.
Can we also add a test that sets the "children" prop explicitly, in both cases?
There was a problem hiding this comment.
Good idea. Added.
There was a problem hiding this comment.
I see you've added it for React.createElement, but not for the jsx example.
There was a problem hiding this comment.
Didn't even think about passing a children prop in JSX. Seems just wrong to even do such thing. Will update the rule.
There was a problem hiding this comment.
It's very wrong but we should still cover it :-)
There was a problem hiding this comment.
Sounds like another good rule would be to prevent a children prop on DOM elements.
There was a problem hiding this comment.
I'd say on any elements - children should be actual children (in React.createElement as well)
petersendidit
force-pushed
the
dom-elements-no-danger-with-children
branch
2 times, most recently
from
8ed6f7e to
8773a33
Compare
|
Anything else needed to get this merged? |
| @@ -0,0 +1,39 @@ | |||
| # Prevent problem with children and props.dangerouslySetInnerHTML (dom-elements-no-danger-with-children) | |||
|
|
|||
| This rule helps prevent problems caused by using children and the dangerouslySetInnerHTML prop at the same time | |||
There was a problem hiding this comment.
This sentence needs ending punctuation.
Also, it might be worth mentioning that React will issue a warning for this case.
petersendidit
force-pushed
the
dom-elements-no-danger-with-children
branch
from
8773a33 to
a239cf2
Compare
|
New commit should take care of the comments. Let me know if there is anything else. |
| docs: { | ||
| description: 'Report when a DOM element is using both children and dangerouslySetInnerHTML', | ||
| category: '', | ||
| recommended: true |
There was a problem hiding this comment.
The rule is not in the recommended configuration (yet).
There was a problem hiding this comment.
It should be, though, since it's already a React error.
|
Thanks, just a few small things:
And then I think we'll be good to go :) |
Prevents dangerouslySetInnerHTML and children from being used at the same time
petersendidit
force-pushed
the
dom-elements-no-danger-with-children
branch
from
a239cf2 to
5fbee0c
Compare
|
Renamed and removed it from the recommended rules. I do think these type of rules that are catching errors that React will throw itself should be recommend ASAP. |
|
Agreed. I think we should write them all, put out minor releases, and then On Sat, Aug 13, 2016, 11:09 AM David Petersen notifications@github.com
|
|
Although, I kind of feel like it's not really a semver-major if React would be throwing the error at runtime anyways. |
|
I think it is a warning |
Yes, it will be added to the recommended config on next major release. |
|
Merges, thanks! |
Prevents dangerouslySetInnerHTML and children from being used at the same time (Fixes #710)