Skip to content
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

Update: add enforceForJSX option to no-unused-expressions rule #14012

Merged
merged 11 commits into from Feb 10, 2021
Merged
23 changes: 23 additions & 0 deletions docs/rules/no-unused-expressions.md
Expand Up @@ -31,6 +31,7 @@ This rule, in its default state, does not require any arguments. If you would li
* `allowShortCircuit` set to `true` will allow you to use short circuit evaluations in your expressions (Default: `false`).
* `allowTernary` set to `true` will enable you to use ternary operators in your expressions similarly to short circuit evaluations (Default: `false`).
* `allowTaggedTemplates` set to `true` will enable you to use tagged template literals in your expressions (Default: `false`).
* `enforceForJSX` set to `true` will flag unused JSX element expressions (Default: `false`).
Copy link
Member

Choose a reason for hiding this comment

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

Sorry @duncanbeevers, I forgot about the examples of correct and incorrect code with each option below. Can you add those for this new option? That'd be a fine place to explain that people can enable this option if JSX has no side effects as in React. (Thanks @mdjermanovic for catching my mistake.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. I considered adding those, but I thought they might be superfluous. I really don't want this PR to add a lot of noise, and I especially want to make sure anything public-facing gets explicit consideration.

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've added examples and a little descriptive text about JSX pragmas. I'm still concerned about being too low-level, and really want something sweet, simple, and that clearly indicates to the 99.9999% of JSX users that they should turn this on.


These options allow unused expressions *only if all* of the code paths either directly change the state (for example, assignment statement) or could have *side effects* (for example, function call).

Expand Down Expand Up @@ -161,3 +162,25 @@ Examples of **correct** code for the `{ "allowTaggedTemplates": true }` option:

tag`some tagged template string`;
```

### enforceForJSX

JSX is most-commonly used in the React ecosystem, where it is compiled to `React.createElement` expressions. Though free from side-effects, these calls are not automatically flagged by the `no-unused-expression` rule. If you're using React, or any other side-effect-free JSX pragma, this option can be enabled to flag these expressions.

Examples of **incorrect** code for the `{ "enforceForJSX": true }` option:

```jsx
/*eslint no-unused-expressions: ["error", { "enforceForJSX": true }]*/

<MyComponent />
<></>
duncanbeevers marked this conversation as resolved.
Show resolved Hide resolved
```

Examples of **correct** code for the `{ "enforceForJSX": true }` option:

```jsx
/*eslint no-unused-expressions: ["error", { "enforceForJSX": true }]*/

var myComponentPartial = <MyComponent />
var myFragment = <></>
duncanbeevers marked this conversation as resolved.
Show resolved Hide resolved
```
13 changes: 12 additions & 1 deletion lib/rules/no-unused-expressions.js
Expand Up @@ -50,6 +50,10 @@ module.exports = {
allowTaggedTemplates: {
type: "boolean",
default: false
},
enforceForJSX: {
type: "boolean",
default: false
}
},
additionalProperties: false
Expand All @@ -65,7 +69,8 @@ module.exports = {
const config = context.options[0] || {},
allowShortCircuit = config.allowShortCircuit || false,
allowTernary = config.allowTernary || false,
allowTaggedTemplates = config.allowTaggedTemplates || false;
allowTaggedTemplates = config.allowTaggedTemplates || false,
enforceForJSX = config.enforceForJSX || false;

// eslint-disable-next-line jsdoc/require-description
/**
Expand Down Expand Up @@ -140,6 +145,12 @@ module.exports = {
},
FunctionExpression: alwaysTrue,
Identifier: alwaysTrue,
JSXElement() {
return enforceForJSX;
},
JSXFragment() {
return enforceForJSX;
},
Literal: alwaysTrue,
LogicalExpression(node) {
if (allowShortCircuit) {
Expand Down
38 changes: 38 additions & 0 deletions tests/lib/rules/no-unused-expressions.js
Expand Up @@ -82,6 +82,30 @@ ruleTester.run("no-unused-expressions", rule, {
{
code: "obj?.foo(\"bar\")",
parserOptions: { ecmaVersion: 11 }
},

// JSX
{
code: "<div />",
parserOptions: { ecmaFeatures: { jsx: true } }
},
{
code: "<></>",
parserOptions: { ecmaFeatures: { jsx: true } }
},
{
code: "var partial = <div />",
parserOptions: { ecmaFeatures: { jsx: true } }
duncanbeevers marked this conversation as resolved.
Show resolved Hide resolved
},
{
code: "var partial = <div />",
options: [{ enforceForJSX: true }],
parserOptions: { ecmaFeatures: { jsx: true } }
},
{
code: "var partial = <></>",
options: [{ enforceForJSX: true }],
parserOptions: { ecmaFeatures: { jsx: true } }
}
],
invalid: [
Expand Down Expand Up @@ -152,6 +176,20 @@ ruleTester.run("no-unused-expressions", rule, {
code: "obj?.foo().bar",
parserOptions: { ecmaVersion: 2020 },
errors: [{ messageId: "unusedExpression", type: "ExpressionStatement" }]
},

// JSX
{
code: "<div />",
options: [{ enforceForJSX: true }],
parserOptions: { ecmaFeatures: { jsx: true } },
errors: [{ messageId: "unusedExpression", type: "ExpressionStatement" }]
},
{
code: "<></>",
options: [{ enforceForJSX: true }],
parserOptions: { ecmaFeatures: { jsx: true } },
errors: [{ messageId: "unusedExpression", type: "ExpressionStatement" }]
}
]
});