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

feat(rules): add jsx-props-no-spread-multi #3724

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Expand Up @@ -338,6 +338,7 @@ module.exports = [
| [jsx-one-expression-per-line](docs/rules/jsx-one-expression-per-line.md) | Require one JSX element per line | | | 🔧 | | |
| [jsx-pascal-case](docs/rules/jsx-pascal-case.md) | Enforce PascalCase for user-defined JSX components | | | | | |
| [jsx-props-no-multi-spaces](docs/rules/jsx-props-no-multi-spaces.md) | Disallow multiple spaces between inline JSX props | | | 🔧 | | |
| [jsx-props-no-spread-multi](docs/rules/jsx-props-no-spread-multi.md) | Disallow JSX prop spreading the same expression multiple times | ☑️ | | | | |
| [jsx-props-no-spreading](docs/rules/jsx-props-no-spreading.md) | Disallow JSX prop spreading | | | | | |
| [jsx-sort-default-props](docs/rules/jsx-sort-default-props.md) | Enforce defaultProps declarations alphabetical sorting | | | | | ❌ |
| [jsx-sort-props](docs/rules/jsx-sort-props.md) | Enforce props alphabetical sorting | | | 🔧 | | |
Expand Down
1 change: 1 addition & 0 deletions configs/recommended.js
Expand Up @@ -11,6 +11,7 @@ module.exports = Object.assign({}, all, {
'react/jsx-no-duplicate-props': 2,
'react/jsx-no-target-blank': 2,
'react/jsx-no-undef': 2,
'react/jsx-props-no-spread-multi': 2,
SimonSchick marked this conversation as resolved.
Show resolved Hide resolved
'react/jsx-uses-react': 2,
'react/jsx-uses-vars': 2,
'react/no-children-prop': 2,
Expand Down
27 changes: 27 additions & 0 deletions docs/rules/jsx-props-no-spread-multi.md
@@ -0,0 +1,27 @@
# Disallow JSX prop spreading the same expression multiple times (`react/jsx-props-no-spread-multi`)

💼 This rule is enabled in the ☑️ `recommended` [config](https://github.com/jsx-eslint/eslint-plugin-react/#shareable-configs).

<!-- end auto-generated rule header -->

Enforces that any unique express is only spread once. Generally spreading the same expression twice is an indicator of a mistake since any attribute between the spreads may be overridden when
the intent was not to. Even when that is not the case this will lead to unnecessary computations to be performed.

## Rule Details

Examples of **incorrect** code for this rule:

```jsx
<App {...props} myAttr='1' {...props} />
Copy link
Author

Choose a reason for hiding this comment

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

TODO: Should I add more expression examples eg. function calls?

```

Examples of **correct** code for this rule:

```jsx
<App myAttr='1' {...props} />
<App {...props} myAttr='1' />
```

## When Not To Use It

When spreading the same expression yields different values.
1 change: 1 addition & 0 deletions lib/rules/index.js
Expand Up @@ -50,6 +50,7 @@ module.exports = {
'jsx-fragments': require('./jsx-fragments'),
'jsx-props-no-multi-spaces': require('./jsx-props-no-multi-spaces'),
'jsx-props-no-spreading': require('./jsx-props-no-spreading'),
'jsx-props-no-spread-multi': require('./jsx-props-no-spread-multi'),
'jsx-sort-default-props': require('./jsx-sort-default-props'),
'jsx-sort-props': require('./jsx-sort-props'),
'jsx-space-before-closing': require('./jsx-space-before-closing'),
Expand Down
60 changes: 60 additions & 0 deletions lib/rules/jsx-props-no-spread-multi.js
@@ -0,0 +1,60 @@
/**
* @fileoverview Prevent JSX prop spreading the same expression multiple times
* @author Simon Schick
*/

'use strict';

const docsUrl = require('../util/docsUrl');
const report = require('../util/report');

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------

const messages = {
noMultiSpreading: 'Spreading the same expression multiple times is forbidden',
};

/**
* Filter for JSON.stringify that omits circular and position structures.
*
* @param {string} key
* @param {*} value
* @returns {*}
*/
const propertyFilter = (key, value) => (key !== 'parent' && key !== 'range' && key !== 'loc' ? value : undefined);

module.exports = {
meta: {
docs: {
description: 'Disallow JSX prop spreading the same expression multiple times',
category: 'Best Practices',
recommended: true,
url: docsUrl('jsx-props-no-spread-multi'),
},
messages,
},

create(context) {
return {
JSXOpeningElement(node) {
const spreads = node.attributes.filter((attr) => attr.type === 'JSXSpreadAttribute');
if (spreads.length < 2) {
Copy link
Author

Choose a reason for hiding this comment

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

Would be nice to have some stats on how many spreads exists on average per component, though this probably covers ~99.99% of all component instantiations.

return;
}
const hashes = new Set();
for (const spread of spreads) {
// TODO: Deep compare ast function?
const hash = JSON.stringify(spread, propertyFilter);
Copy link
Author

Choose a reason for hiding this comment

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

This is admittedly somewhat of a bandaid.
I used a various of deepEqual before to compare all possible matches (which is also O(n^2)).
This performs pretty well and generally has been generating small 'hashes' however I am worried of scenarios where developers create excessively large/syntactically complex spreads.

In these cases we could perform a simple md5/sha hashing though this may be overkill.

Copy link
Author

Choose a reason for hiding this comment

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

Optimized it a little by only hashing the argument rather than the full node.

if (hashes.has(hash)) {
report(context, messages.noMultiSpreading, 'noMultiSpreading', {
node: spread,
});
}
hashes.add(hash);
}
},
};
},
};
9 changes: 7 additions & 2 deletions lib/types.d.ts
Expand Up @@ -11,11 +11,16 @@ declare global {
type JSXAttribute = ASTNode;
type JSXElement = ASTNode;
type JSXFragment = ASTNode;
type JSXOpeningElement = ASTNode;
type JSXSpreadAttribute = ASTNode;

type Context = eslint.Rule.RuleContext
type Context = eslint.Rule.RuleContext;

type TypeDeclarationBuilder = (annotation: ASTNode, parentName: string, seen: Set<typeof annotation>) => object;
type TypeDeclarationBuilder = (
Copy link
Author

Choose a reason for hiding this comment

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

This was auto-formatted, I assume this is ok?

annotation: ASTNode,
parentName: string,
seen: Set<typeof annotation>
) => object;

type TypeDeclarationBuilders = {
[k in string]: TypeDeclarationBuilder;
Expand Down
71 changes: 71 additions & 0 deletions tests/lib/rules/jsx-props-no-spread-multi.js
@@ -0,0 +1,71 @@
/**
* @fileoverview Tests for jsx-props-no-spread-multi
*/

'use strict';

// -----------------------------------------------------------------------------
// Requirements
// -----------------------------------------------------------------------------

const RuleTester = require('eslint').RuleTester;
const rule = require('../../../lib/rules/jsx-props-no-spread-multi');

const parsers = require('../../helpers/parsers');

const parserOptions = {
ecmaVersion: 2018,
sourceType: 'module',
ecmaFeatures: {
jsx: true,
},
};

// -----------------------------------------------------------------------------
// Tests
// -----------------------------------------------------------------------------

const ruleTester = new RuleTester({ parserOptions });
const expectedError = { messageId: 'noMultiSpreading' };

ruleTester.run('jsx-props-no-spread-multi', rule, {
valid: parsers.all([
{
code: `
const a = {};
<App {...a} />
`,
},
{
code: `
const a = {};
const b = {};
<App {...a} {...b} />
`,
},
]),

invalid: parsers.all([
{
code: `
const props = {};
<App {...props} {...props} />
`,
errors: [expectedError],
},
{
code: `
const props = {};
<div {...props} a='a' {...props} />
`,
errors: [expectedError],
},
{
code: `
const func = () => ({});
<div {...func()} {...func()} />
`,
errors: [expectedError],
},
]),
});