Skip to content

[New] jsx-boolean-value: add inverse option for always/never #1250

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 3 commits into from
Jun 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
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
12 changes: 7 additions & 5 deletions docs/rules/jsx-boolean-value.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,29 @@

## Rule Details

This rule takes one argument. If it is `"always"` then it warns whenever an attribute is missing its value. If `"never"` then it warns if an attribute has a `true` value. The default value of this option is `"never"`.
This rule takes two arguments. If the first argument is `"always"` then it warns whenever an attribute is missing its value. If `"never"` then it warns if an attribute has a `true` value. The default value of this option is `"never"`.

The following patterns are considered warnings when configured `"never"`:
The second argument is optional: if provided, it must be an object with a `"never"` property (if the first argument is `"always"`), or an `"always"` property (if the first argument is `"never"`). This property’s value must be an array of strings representing prop names.

The following patterns are considered warnings when configured `"never"`, or with `"always", { "never": ["personal"] }`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm on the fence about whether this should be never/always, or something more general like exceptions or except. As it is written here, it is possible to have both never and always which would be a little weird. I think it is fine, but I think a unified option might make the documentation a little clearer and the code a small amount simpler. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, "exceptions" means "these things are not checked" - the semantic here is, everything is forced to something: some things are forced to always, and some to never.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see. That makes sense.


```jsx
var Hello = <Hello personal={true} />;
```

The following patterns are not considered warnings when configured `"never"`:
The following patterns are not considered warnings when configured `"never"`, or with `"always", { "never": ["personal"] }`:

```jsx
var Hello = <Hello personal />;
```

The following patterns are considered warnings when configured `"always"`:
The following patterns are considered warnings when configured `"always"`, or with `"never", { "always": ["personal"] }`:

```jsx
var Hello = <Hello personal />;
```

The following patterns are not considered warnings when configured `"always"`:
The following patterns are not considered warnings when configured `"always"`, or with `"never", { "always": ["personal"] }`:

```jsx
var Hello = <Hello personal={true} />;
Expand Down
126 changes: 95 additions & 31 deletions lib/rules/jsx-boolean-value.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,41 @@
// Rule Definition
// ------------------------------------------------------------------------------

const exceptionsSchema = {
type: 'array',
items: {type: 'string', minLength: 1},
uniqueItems: true
};

const ALWAYS = 'always';
const NEVER = 'never';

const errorData = new WeakMap();
function getErrorData(exceptions) {
if (!errorData.has(exceptions)) {
const exceptionProps = Array.from(exceptions, (name) => `\`${name}\``).join(', ');
const exceptionsMessage = exceptions.size > 0 ? ` for the following props: ${exceptionProps}` : '';
errorData.set(exceptions, {exceptionsMessage: exceptionsMessage});
}
return errorData.get(exceptions);
}

function isAlways(configuration, exceptions, propName) {
const isException = exceptions.has(propName);
if (configuration === ALWAYS) {
return !isException;
}
return isException;
}

function isNever(configuration, exceptions, propName) {
const isException = exceptions.has(propName);
if (configuration === NEVER) {
return !isException;
}
return isException;
}

module.exports = {
meta: {
docs: {
Expand All @@ -17,44 +52,73 @@ module.exports = {
},
fixable: 'code',

schema: [{
enum: ['always', 'never']
}]
schema: {
anyOf: [{
type: 'array',
items: [{enum: [ALWAYS, NEVER]}],
additionalItems: false
}, {
type: 'array',
items: [{
enum: [ALWAYS]
}, {
type: 'object',
additionalProperties: false,
properties: {
[NEVER]: exceptionsSchema
}
}],
additionalItems: false
}, {
type: 'array',
items: [{
enum: [NEVER]
}, {
type: 'object',
additionalProperties: false,
properties: {
[ALWAYS]: exceptionsSchema
}
}],
additionalItems: false
}]
}
},

create: function(context) {
var configuration = context.options[0] || 'never';
create(context) {
const configuration = context.options[0] || NEVER;
const configObject = context.options[1] || {};
const exceptions = new Set((configuration === ALWAYS ? configObject[NEVER] : configObject[ALWAYS]) || []);

var NEVER_MESSAGE = 'Value must be omitted for boolean attributes';
var ALWAYS_MESSAGE = 'Value must be set for boolean attributes';
const NEVER_MESSAGE = 'Value must be omitted for boolean attributes{{exceptionsMessage}}';
const ALWAYS_MESSAGE = 'Value must be set for boolean attributes{{exceptionsMessage}}';
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be nice to move some of this into a helper function that takes exceptions as an argument, so this code only has to run in the event of a warning.


return {
JSXAttribute: function(node) {
switch (configuration) {
case 'always':
if (node.value === null) {
context.report({
node: node,
message: ALWAYS_MESSAGE,
fix: function(fixer) {
return fixer.insertTextAfter(node, '={true}');
}
});
JSXAttribute(node) {
const propName = node.name && node.name.name;
const value = node.value;

if (isAlways(configuration, exceptions, propName) && value === null) {
const data = getErrorData(exceptions);
context.report({
node: node,
message: ALWAYS_MESSAGE,
data: data,
fix(fixer) {
return fixer.insertTextAfter(node, '={true}');
}
break;
case 'never':
if (node.value && node.value.type === 'JSXExpressionContainer' && node.value.expression.value === true) {
context.report({
node: node,
message: NEVER_MESSAGE,
fix: function(fixer) {
return fixer.removeRange([node.name.range[1], node.value.range[1]]);
}
});
});
}
if (isNever(configuration, exceptions, propName) && value && value.type === 'JSXExpressionContainer' && value.expression.value === true) {
const data = getErrorData(exceptions);
context.report({
node: node,
message: NEVER_MESSAGE,
data: data,
fix(fixer) {
return fixer.removeRange([node.name.range[1], value.range[1]]);
}
break;
default:
break;
});
}
}
};
Expand Down
30 changes: 26 additions & 4 deletions tests/lib/rules/jsx-boolean-value.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,42 @@ var ruleTester = new RuleTester({parserOptions});
ruleTester.run('jsx-boolean-value', rule, {
valid: [
{code: '<App foo />;', options: ['never']},
{code: '<App foo bar={true} />;', options: ['always', {never: ['foo']}]},
{code: '<App foo />;'},
{code: '<App foo={true} />;', options: ['always']}
{code: '<App foo={true} />;', options: ['always']},
{code: '<App foo={true} bar />;', options: ['never', {always: ['foo']}]}
],
invalid: [{
code: '<App foo={true} />;', output: '<App foo />;', options: ['never'],
code: '<App foo={true} />;',
output: '<App foo />;',
options: ['never'],
errors: [{message: 'Value must be omitted for boolean attributes'}]
}, {
code: '<App foo={true} />;', output: '<App foo />;',
code: '<App foo={true} bar={true} baz={true} />;',
output: '<App foo bar baz={true} />;',
options: ['always', {never: ['foo', 'bar']}],
errors: [
{message: 'Value must be omitted for boolean attributes for the following props: `foo`, `bar`'},
{message: 'Value must be omitted for boolean attributes for the following props: `foo`, `bar`'}
]
}, {
code: '<App foo={true} />;',
output: '<App foo />;',
errors: [{message: 'Value must be omitted for boolean attributes'}]
}, {
code: '<App foo = {true} />;', output: '<App foo />;',
code: '<App foo = {true} />;',
output: '<App foo />;',
errors: [{message: 'Value must be omitted for boolean attributes'}]
}, {
code: '<App foo />;', output: '<App foo={true} />;', options: ['always'],
errors: [{message: 'Value must be set for boolean attributes'}]
}, {
code: '<App foo bar baz />;',
output: '<App foo={true} bar={true} baz />;',
options: ['never', {always: ['foo', 'bar']}],
errors: [
{message: 'Value must be set for boolean attributes for the following props: `foo`, `bar`'},
{message: 'Value must be set for boolean attributes for the following props: `foo`, `bar`'}
]
}]
});