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: mapStateToProps-prefer-selectors #50

Merged
merged 4 commits into from
Jul 13, 2020
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,6 @@ To configure individual rules:
* [react-redux/mapStateToProps-no-store](docs/rules/mapStateToProps-no-store.md) Prohibits binding a whole store object to a component.
* [react-redux/mapStateToProps-prefer-hoisted](docs/rules/mapStateToProps-prefer-hoisted.md) Flags generation of copies of same-by-value but different-by-reference props.
* [react-redux/mapStateToProps-prefer-parameters-names](docs/rules/mapStateToProps-prefer-parameters-names.md) Enforces that all mapStateToProps parameters have specific names.
* [react-redux/mapStateToProps-prefer-selectors](docs/rules/mapStateToProps-prefer-selectors.md) Enforces that all mapStateToProps properties use selector functions.
* [react-redux/no-unused-prop-types](docs/rules/no-unused-prop-types.md) Extension of a react's no-unused-prop-types rule filtering out false positive used in redux context.
* [react-redux/prefer-separate-component-file](docs/rules/prefer-separate-component-file.md) Enforces that all connected components are defined in a separate file.
94 changes: 94 additions & 0 deletions docs/rules/mapStateToProps-prefer-selectors.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
# Enforces that all mapStateToProps properties use selector functions. (react-redux/mapStateToProps-prefer-selectors)

Using selectors in `mapStateToProps` to pull data from the store or [compute derived data](https://redux.js.org/recipes/computing-derived-data#composing-selectors) allows you to uncouple your containers from the state architecture and more easily enable memoization. This rule will ensure that every prop utilizes a selector.

## Rule details

The following pattern is considered incorrect:

```js
const mapStateToProps = (state) => { x: state.property }
```

```js
connect(function(state) {
return {
y: state.other.property
}
}, null)(App)
```

The following patterns are considered correct:

```js
const propertySelector = (state) => state.property
const mapStateToProps = (state) => { x: propertySelector(state) }
```

```js
const getOtherProperty = (state) => state.other.property
connect(function(state) {
return {
y: getOtherProperty(state)
}
}, null)(App)
```

## Rule Options

```js
...
"react-redux/mapStateToProps-prefer-selectors": [<enabled>, {
"matching": <string>
"validateParams": <boolean>
}]
...
```

### `matching`
If provided, validates the name of the selector functions against the RegExp pattern provided.
Copy link
Owner

Choose a reason for hiding this comment

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

would be nice to provide some examples of correct/incorrect for illustration, you have quite a few in tests.


```js
// .eslintrc
{
"react-redux/mapStateToProps-prefer-selectors": ["error", { matching: "^.*Selector$"}]
}

// container.js
const mapStateToProps = (state) => {
x: xSelector(state), // success
y: selectY(state), // failure
}
```

```js
// .eslintrc
{
"react-redux/mapStateToProps-prefer-selectors": ["error", { matching: "^get.*FromState$"}]
}

// container.js
const mapStateToProps = (state) => {
x: getXFromState(state), // success
y: getY(state), // failure
}
```

### `validateParams`
Boolean to determine if the selectors use the correct params (`<selectorFunction>(state, ownProps)`, where both params are optional). Defaults to true.

```js
// .eslintrc
{
"react-redux/mapStateToProps-prefer-selectors": ["error", { validateParams: true }]
}

// container.js
const mapStateToProps = (state, ownProps) => {
x: xSelector(state), // success
y: ySelector(state, ownProps), // sucess
z: zSelector(), // success
a: aSelector(ownProps, state), // failure
b: bSelector(state, someOtherValue) // failure
}
```
1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const rules = {
'mapStateToProps-no-store': require('./lib/rules/mapStateToProps-no-store'),
'mapStateToProps-prefer-hoisted': require('./lib/rules/mapStateToProps-prefer-hoisted'),
'mapStateToProps-prefer-parameters-names': require('./lib/rules/mapStateToProps-prefer-parameters-names'),
'mapStateToProps-prefer-selectors': require('./lib/rules/mapStateToProps-prefer-selectors'),
'no-unused-prop-types': require('./lib/rules/no-unused-prop-types'),
'prefer-separate-component-file': require('./lib/rules/prefer-separate-component-file'),
};
Expand Down
97 changes: 97 additions & 0 deletions lib/rules/mapStateToProps-prefer-selectors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
const isReactReduxConnect = require('../isReactReduxConnect');
const utils = require('../utils');

const reportNoSelector = function (context, node, name) {
context.report({
message: `mapStateToProps property "${name}" should use a selector function.`,
node,
});
};

const reportWrongName = function (context, node, propName, functionName, matching) {
context.report({
message: `mapStateToProps "${propName}"'s selector "${functionName}" does not match "${matching}".`,
node,
});
};

const reportUnexpectedParam = function (context, node, propName, functionName, index) {
context.report({
message: `mapStateToProps "${propName}"'s selector "${functionName}" parameter #${index} is not expected.`,
node,
});
};

const reportInvalidParams = function (context, node, propName, functionName, params, index) {
context.report({
message: `mapStateToProps "${propName}"'s selector "${functionName}" parameter #${index} should be "${params[index].name}".`,
node,
});
};

const checkProperties = function (context, properties, matching, expectedParams) {
properties.forEach((prop) => {
if (prop.value.type !== 'CallExpression') {
reportNoSelector(context, prop, prop.key.name);
return;
}
if (matching && !prop.value.callee.name.match(new RegExp(matching))) {
reportWrongName(context, prop, prop.key.name, prop.value.callee.name, matching);
}
if (expectedParams) {
const actualParams = prop.value.arguments;
const propName = prop.key.name;
const functionName = prop.value.callee.name;
actualParams.forEach((param, i) => {
if (!expectedParams[i]) {
reportUnexpectedParam(context, prop, propName, functionName, i);
return;
}
if (param.name !== expectedParams[i].name) {
reportInvalidParams(context, prop, propName, functionName, expectedParams, i);
}
});
}
});
};

const check = function (context, node, matching, validateParams) {
const returnNode = utils.getReturnNode(node);
if (utils.isObject(returnNode)) {
checkProperties(context, returnNode.properties, matching, validateParams && node.params);
}
};

module.exports = function (context) {
const config = context.options[0] || {};
return {
VariableDeclaration(node) {
node.declarations.forEach((decl) => {
if (decl.id && decl.id.name === 'mapStateToProps') {
if (decl.init && (
decl.init.type === 'ArrowFunctionExpression' ||
decl.init.type === 'FunctionExpression'
)) {
check(context, decl.init, config.matching, !(config.validateParams === false));
}
}
});
},
FunctionDeclaration(node) {
if (node.id && node.id.name === 'mapStateToProps') {
check(context, node.body, config.matching, !(config.validateParams === false));
}
},
CallExpression(node) {
if (isReactReduxConnect(node)) {
const mapStateToProps = node.arguments && node.arguments[0];
if (mapStateToProps && (
mapStateToProps.type === 'ArrowFunctionExpression' ||
mapStateToProps.type === 'FunctionExpression')
) {
check(context, mapStateToProps, config.matching, !(config.validateParams === false));
}
}
},
};
};
186 changes: 186 additions & 0 deletions tests/lib/rules/mapStateToProps-prefer-selectors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
require('babel-eslint');

const rule = require('../../../lib/rules/mapStateToProps-prefer-selectors');
const RuleTester = require('eslint').RuleTester;

const parserOptions = {
ecmaVersion: 6,
sourceType: 'module',
ecmaFeatures: {
experimentalObjectRestSpread: true,
},
};

const ruleTester = new RuleTester({ parserOptions });

ruleTester.run('mapStateToProps-prefer-selectors', rule, {
valid: [
'const mapStateToProps = (state) => 1',
'const mapStateToProps = (state) => ({})',
'const mapStateToProps = (state) => ({ x: xSelector(state) })',
'const mapStateToProps = (state, ownProps) => ({ x: xSelector(state, ownProps) })',
'const mapStateToProps = (state) => ({ x: xSelector(state), y: ySelector(state) })',
'const mapStateToProps = (state) => { return { x: xSelector(state) }; }',
'const mapStateToProps = (state) => { doSomethingElse(); return { x: xSelector(state) }; }',
'const mapStateToProps = function(state) { return { x: xSelector(state) }; }',
'function mapStateToProps(state) { doSomethingElse(); return { x: xSelector(state) }; }',
'connect((state) => ({ x: xSelector(state) }), {})(Comp)',
'const mapStateToProps = () => ({ x: xSelector() })',
'const mapStateToProps = function(state) { return { x: getX() }; }',
'const mapStateToProps = function(state) { return { x: getX(state) }; }',
'connect((state, ownProps) => ({ x: selector() }), {})(Comp)',
'connect((state, ownProps) => ({ x: selector(state) }), {})(Comp)',
'connect((state, ownProps) => ({ x: selector(state, ownProps) }), {})(Comp)',
{
code: 'const mapStateToProps = (state) => ({ x: xSelector(state) })',
options: [{
matching: '^.*Selector$',
}],
},
{
code: 'const mapStateToProps = function(state) { return { x: getX(state) }; }',
options: [{
matching: '^get.*$',
}],
},
{
code: 'connect((state) => ({ x: selector(state) }), {})(Comp)',
options: [{
matching: '^selector$',
}],
},
{
code: 'const mapStateToProps = (state) => ({ x: xSelector(differentParam) })',
options: [{
validateParams: false,
}],
},
{
code: 'const mapStateToProps = function(state) { return { x: getX(state, ownProps2) }; }',
options: [{
validateParams: false,
}],
},
{
code: 'connect(() => ({ x: selector(state) }), {})(Comp)',
options: [{
validateParams: false,
}],
},
],
invalid: [{
code: 'const mapStateToProps = (state) => ({ x: state.b })',
errors: [
{
message: 'mapStateToProps property "x" should use a selector function.',
},
],
}, {
code: 'const mapStateToProps = (state) => ({ x: state.x, y: state.y })',
errors: [
{
message: 'mapStateToProps property "x" should use a selector function.',
},
{
message: 'mapStateToProps property "y" should use a selector function.',
},
],
}, {
code: 'const mapStateToProps = (state) => ({ x: state.x, y: ySelector(state) })',
errors: [
{
message: 'mapStateToProps property "x" should use a selector function.',
},
],
}, {
code: 'const mapStateToProps = (state) => { return { x: state.b }; }',
errors: [
{
message: 'mapStateToProps property "x" should use a selector function.',
},
],
}, {
code: 'const mapStateToProps = (state) => { doSomethingElse(); return { x: state.b }; }',
errors: [
{
message: 'mapStateToProps property "x" should use a selector function.',
},
],
}, {
code: 'const mapStateToProps = function(state) { return { x: state.x }; }',
errors: [
{
message: 'mapStateToProps property "x" should use a selector function.',
},
],
}, {
code: 'function mapStateToProps(state) { doSomethingElse(); return { x: state.b }; }',
errors: [
{
message: 'mapStateToProps property "x" should use a selector function.',
},
],
}, {
code: 'connect((state) => ({ x: state.x }), {})(Comp)',
errors: [
{
message: 'mapStateToProps property "x" should use a selector function.',
},
],
}, {
code: 'const mapStateToProps = (state) => ({ x: xSelector(state) })',
options: [{
matching: '^get.*$',
}],
errors: [{
message: 'mapStateToProps "x"\'s selector "xSelector" does not match "^get.*$".',
}],
}, {
code: 'const mapStateToProps = function(state) { return { x: getX(state) }; }',
options: [{
matching: '^.*Selector$',
}],
errors: [{
message: 'mapStateToProps "x"\'s selector "getX" does not match "^.*Selector$".',
}],
}, {
code: 'connect((state) => ({ x: selectorr(state) }), {})(Comp)',
options: [{
matching: '^selector$',
}],
errors: [{
message: 'mapStateToProps "x"\'s selector "selectorr" does not match "^selector$".',
}],
}, {
code: 'const mapStateToProps = (state) => ({ x: xSelector(state, ownProps) })',
errors: [{
message: 'mapStateToProps "x"\'s selector "xSelector" parameter #1 is not expected.',
}],
}, {
code: 'const mapStateToProps = (state, ownProps) => ({ x: xSelector(state, ownProps, someOtherValue) })',
errors: [{
message: 'mapStateToProps "x"\'s selector "xSelector" parameter #2 is not expected.',
}],
}, {
code: 'const mapStateToProps = function(state) { return { x: getX(notState) }; }',
errors: [{
message: 'mapStateToProps "x"\'s selector "getX" parameter #0 should be "state".',
}],
}, {
code: 'connect((state, ownProps) => ({ x: getX(state, notOwnProps) }), {})(Comp)',
errors: [{
message: 'mapStateToProps "x"\'s selector "getX" parameter #1 should be "ownProps".',
}],
}, {
code: 'connect((state2, ownProps) => ({ x: getX(state) }), {})(Comp)',
errors: [{
message: 'mapStateToProps "x"\'s selector "getX" parameter #0 should be "state2".',
}],
}, {
code: 'connect((state, ownProps2) => ({ x: getX(state, ownProps) }), {})(Comp)',
errors: [{
message: 'mapStateToProps "x"\'s selector "getX" parameter #1 should be "ownProps2".',
}],
}],

});