Rule for requiring HTML entities to be escaped #681
Conversation
| code: [ | ||
| 'class Comp1 extends Component {', | ||
| ' render() {', | ||
| ' return <div>Multiple errors: \'>></div>;', |
There was a problem hiding this comment.
can we also ensure that {'>'} is allowed, by adding a test for it?
There was a problem hiding this comment.
the test on line 70 should cover that case
There was a problem hiding this comment.
whoops, missed that. thanks, that works.
| '}' | ||
| ].join('\n'), | ||
| args: [1], | ||
| parser: 'babel-eslint', |
There was a problem hiding this comment.
wait, why is babel-eslint required? the default parser should be able to handle all of these examples.
There was a problem hiding this comment.
Okay - I can remove this. I copied this from another test and didn't know it could be removed
There was a problem hiding this comment.
if it works when removed, then i'd say please do :-) if not, then obv you should keep it
There was a problem hiding this comment.
Hm, I tried removing it and now all the tests fail with "Parsing error: Unexpected token <"
Is that correct? how is the JSX compiled if babel-eslint is not used?
There was a problem hiding this comment.
ah - we may need to enable the jsx parser option.
There was a problem hiding this comment.
Is that something I can do? I tried setting parser: 'jsx' but that didn't work
There was a problem hiding this comment.
There was a problem hiding this comment.
great thanks! the latest version should use the correct parser
|
Any thoughts on this rule? Happy to make any recommended updates. We've been running this in development and have already gotten a lot of value out of it. It has caught several errors around mistyped closing tags cc @yannickcr |
| // Rule Definition | ||
| // ------------------------------------------------------------------------------ | ||
|
|
||
| var DEFAULTS = ['>', '<', '"', '\'', '}']; |
There was a problem hiding this comment.
I see the value in linting for >, since you have provided such a good example of what that can be problematic. However, I'm not so sure about the other characters listed in the defaults here. Can you provide similar reasoning for each of these characters?
There was a problem hiding this comment.
I think <<div> is just as problematic as </div>>, and {<Foo />}} and {{<Foo />} so those seem obvious to me (let's add {?)
I'm not sure I see the purpose in having double or single quotes in here, though.
There was a problem hiding this comment.
Yeah, I also found the omission of { curious. In any case, it would be really great if the documentation included an example of why each of the defaults can be problematic.
There was a problem hiding this comment.
I omitted { because it's not actually possible to include this character inside a tag - this character begins a curly-brace section and is a syntax error. I could include it here for completeness but AFAICT would have no effect.
This does raise a good point that < is also extraneous, I should remove that.
Here's an example where quotes can bite you:
<Foo
a="b"
c="d">
e="f"Perhaps you get into this state by sorting props alphabetically (this is where it has happened to me). I'm happy to update the documentation to include this example as well
There was a problem hiding this comment.
I've added examples for all the cases to the documentation.
|
@pfhayes please rebase so as to remove merge commits from the PR :-) |
|
I've squashed this into a single commit |
| // included accidentally. | ||
| var DEFAULTS = ['>', '"', '\'', '}']; | ||
|
|
||
| module.exports = function(context) { |
There was a problem hiding this comment.
This should use the new eslint rule format
|
This looks good. Thanks for your contribution! |
I will also add in some documentation around this, but wanted to get this out there to make sure I was on the right track
I ran this on our codebase and it caught a bunch of errors so it appears to work