Skip to content

Add auto fix for self-closing-comp #770

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

Merged
merged 5 commits into from
Aug 20, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ Finally, enable all of the rules that you would like to use. Use [our preset](#
* [react/require-extension](docs/rules/require-extension.md): Restrict file extensions that may be required
* [react/require-optimization](docs/rules/require-optimization.md): Enforce React components to have a shouldComponentUpdate method
* [react/require-render-return](docs/rules/require-render-return.md): Enforce ES5 or ES6 class for returning value in render function
* [react/self-closing-comp](docs/rules/self-closing-comp.md): Prevent extra closing tags for components without children
* [react/self-closing-comp](docs/rules/self-closing-comp.md): Prevent extra closing tags for components without children (fixable)
* [react/sort-comp](docs/rules/sort-comp.md): Enforce component methods order
* [react/sort-prop-types](docs/rules/sort-prop-types.md): Enforce propTypes declarations alphabetical sorting

Expand Down
2 changes: 2 additions & 0 deletions docs/rules/self-closing-comp.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

Components without children can be self-closed to avoid unnecessary extra closing tag.

**Fixable:** This rule is automatically fixable using the `--fix` flag on the command line.

## Rule Details

The following patterns are considered warnings:
Expand Down
13 changes: 12 additions & 1 deletion lib/rules/self-closing-comp.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module.exports = {
category: 'Stylistic Issues',
recommended: false
},
fixable: 'code',

schema: [{
type: 'object',
Expand Down Expand Up @@ -75,7 +76,17 @@ module.exports = {
}
context.report({
node: node,
message: 'Empty components are self-closing'
message: 'Empty components are self-closing',
fix: function(fixer) {
// Represents the last character of the JSXOpeningElement, the '>' character
var openingElementEnding = node.end - 1;
// Represents the last character of the JSXClosingElement, the '>' character
var closingElementEnding = node.parent.closingElement.end;

// Replace />.*<\/.*>/ with '/>'
var range = [openingElementEnding, closingElementEnding];
return fixer.replaceTextRange(range, '/>');
}
});
}
};
Expand Down
11 changes: 11 additions & 0 deletions tests/lib/rules/self-closing-comp.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,73 +107,84 @@ ruleTester.run('self-closing-comp', rule, {
invalid: [
{
code: 'var contentContainer = <div className="content"></div>;',
output: 'var contentContainer = <div className="content"/>;',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should all autofix with a space before the />.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand, as that change makes the tests fail. It seems that if there is no space before the > then after fixing there should continue to be no space before the >.

Would you prefer that the replacing value located in lib/rules/self-closing-comp.js - Line 88 be changed from "/>" to " />"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly what i'm suggesting. <div /> is correct, not <div/>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for clarifying, I didn't realize the white space was part of the rule. I have added that behavior now. Do you specifically want to enforce a single white space character, or would multiple be valid? For example, would the following be invalid:

<Foo  /> // double white space

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think multiple would be ok, but we'd want to encourage single.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that is the only ordering possible, because the jsx-space-before-closing rule does not produce a warning unless there is a self-closing tag. This obviously cannot happen if self-closing-comp is in violation, because that rule implies there is no self-closing tag. Therefore I think it is impossible for jsx-space-before-closing to produce an error on the same node as self-closing-comp, and the warning conditions for the two rules are mutually exclusive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but someone may not have enabled jsx-space-before-closing - I want the burden of running that extra autofixer to be on someone who has chosen "never" - which is a very unconventional choice - not on the vast majority of JSX authors who always put a space there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, in that case I can re-commit the white space character before a self-closing tag. Should there be any other behavior for white space than what is in f6cc36a?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, that looks fine to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, let me know if you want commits squashed before merging.

parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var contentContainer = <div className="content"></div>;',
output: 'var contentContainer = <div className="content"/>;',
options: [],
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var HelloJohn = <Hello name="John"></Hello>;',
output: 'var HelloJohn = <Hello name="John"/>;',
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var HelloJohn = <Hello name="John">\n</Hello>;',
output: 'var HelloJohn = <Hello name="John"/>;',
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var HelloJohn = <Hello name="John"> </Hello>;',
output: 'var HelloJohn = <Hello name="John"/>;',
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
},
{
code: 'var HelloJohn = <Hello name="John"></Hello>;',
output: 'var HelloJohn = <Hello name="John"/>;',
options: [],
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var HelloJohn = <Hello name="John">\n</Hello>;',
output: 'var HelloJohn = <Hello name="John"/>;',
options: [],
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var HelloJohn = <Hello name="John"> </Hello>;',
output: 'var HelloJohn = <Hello name="John"/>;',
options: [],
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var contentContainer = <div className="content"></div>;',
output: 'var contentContainer = <div className="content"/>;',
options: [{html: true}],
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var contentContainer = <div className="content">\n</div>;',
output: 'var contentContainer = <div className="content"/>;',
options: [{html: true}],
parserOptions: parserOptions,
errors: [{
message: 'Empty components are self-closing'
}]
}, {
code: 'var contentContainer = <div className="content"> </div>;',
output: 'var contentContainer = <div className="content"/>;',
options: [{html: true}],
parserOptions: parserOptions,
errors: [{
Expand Down