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

New: prefer-destructuring rule (fixes #6053) #7741

Merged
Merged
1 change: 1 addition & 0 deletions conf/eslint.json
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@
"padded-blocks": "off",
"prefer-arrow-callback": "off",
"prefer-const": "off",
"prefer-destructuring": "off",
"prefer-numeric-literals": "off",
"prefer-reflect": "off",
"prefer-rest-params": "off",
Expand Down
61 changes: 61 additions & 0 deletions docs/rules/prefer-destructuring.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# Prefer destructuring from arrays and objects (prefer-destructuring)

With JavaScript ES6, a new syntax was added for creating variables from an array index of object property, called [destructuring](#further-reading). This rule enforces usage of destructuring instead of accessing a property through a member expression.
Copy link
Member

Choose a reason for hiding this comment

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

Should this be "from an array index or object property"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, thanks!


## Rule Details
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a paragraph that array destructuring is only enforced if array index is a literal? The last "correct" example is slightly confusing without this note. As in, this rule will flag this code: var a = array[100]; but will not flag this code: var a = array[myVar];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about just expanding the list of "correct" examples to look like:

// With `array` enabled
var [ foo ] = array;
var foo = array[someIndex];

Copy link
Member

@ilyavolodin ilyavolodin Dec 15, 2016

Choose a reason for hiding this comment

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

That sounds good too.


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

```javascript
// With `array` enabled
var foo = array[0];

// With `object` enabled
var foo = object.foo;
var foo = object['foo'];
```

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

```javascript
// With `array` enabled
var [ foo ] = array;

// With `object` enabled
var { foo } = object;
```

### Options

This rule takes two properties, `array` and `object`, which can be used to turn on or off the destructuring requirement for each of those types independently. By default, both are `true`. If you want to change the behavior, you can configure the rule like so:
Copy link
Member

Choose a reason for hiding this comment

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

We should also add documentation for the requireRenaming option (although it would also be fine to wait and see if we plan to bikeshed on the name).


```json
{
"rules": {
"prefer-destructuring": ["error", {
"array": true,
"object": true
}]
}
}
```

## When Not To Use It

If you want to be able to access array indices or object properties directly, you can either configure the rule to your tastes or disable the rule entirely.

Additionally, if you intend to access large array indices directly, like:

```javascript
var foo = array[100];
```

Then the `array` part of this rule is not recommended, as destructuring does not match this use case very well.


## Further Reading

If you want to learn more about destructuring, check out the links below:

- [Destructuring Assignment (MDN)](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment)
- [Destructuring and parameter handling in ECMAScript 6 (2ality blog)](http://www.2ality.com/2015/01/es6-destructuring.html)
165 changes: 165 additions & 0 deletions lib/rules/prefer-destructuring.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
/**
* @fileoverview Prefer destructuring from arrays and objects
* @author Alex LaFroscia
*/
"use strict";

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

module.exports = {
meta: {
docs: {
description: "require destructuring from arrays and/or objects",
category: "ECMAScript 6",
recommended: false
},

schema: [
{
type: "object",
properties: {
array: {
type: "boolean"
},
object: {
type: "boolean"
}
},
additionalProperties: false
},
{
type: "object",
properties: {
requireRenaming: {
type: "boolean"
}
},
additionalProperties: false
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add additionalProperties: false to this object? That way the rule won't silently do the wrong thing if the user makes a typo, e.g. prefer-destructuring: [error, {ojbect: false}].

]
},
create(context) {

let checkArrays = true;
let checkObjects = true;
let requireRenaming = true;
const enabledTypes = context.options[0];
const additionalOptions = context.options[1];

if (enabledTypes) {
if (typeof enabledTypes.array !== "undefined") {
checkArrays = enabledTypes.array;
}

if (typeof enabledTypes.object !== "undefined") {
checkObjects = enabledTypes.object;
}
}

if (additionalOptions) {
if (typeof additionalOptions.requireRenaming !== "undefined") {
requireRenaming = additionalOptions.requireRenaming;
}
}

//--------------------------------------------------------------------------
// Helpers
//--------------------------------------------------------------------------

/**
* Determines if the given node node is accessing an array index
*
* This is used to differentiate array index access from object property
* access.
*
* @param {ASTNode} node the node to evaluate
* @returns {boolean} whether or not the node is an integer
*/
function isArrayIndexAccess(node) {
return Number.isInteger(node.property.value);
Copy link
Member

Choose a reason for hiding this comment

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

This check looks fine and it's correct behavior, but as far as usability is concerned, I'm thinking it might be annoying to get a warning for code like this:

const foo = array[100];

The correct fix for that code would be:

const [ , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , foo] = array;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not really sure what the desired solution to this issue would be. I figure if someone is doing that kind of array access in their app, they wouldn't be using the array feature here.

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 added a note to the docs in the "When not to use this" section about accessing large indices directly, and that not playing very nicely with destructuring. Now sure what else we could do, other than setting some arbitrary (or user-configurable) threshold on when to require it.

}

/**
* Check that the `prefer-destructuring` rules are followed based on the
* given left- and right-hand side of the assignment.
*
* Pulled out into a separate method so that VariableDeclarators and
* AssignmentExpressions can share the same verification logic.
*
* @param {ASTNode} leftNode the left-hand side of the assignment
* @param {ASTNode} rightNode the right-hand side of the assignment
* @param {ASTNode} reportNode the node to report the error on
* @returns {void}
*/
function performCheck(leftNode, rightNode, reportNode) {
if (rightNode.type !== "MemberExpression") {
return;
}

if (checkArrays && isArrayIndexAccess(rightNode)) {
context.report({ node: reportNode, message: "Use array destructuring" });
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: Can you extract the context.report calls into a separate helper function (e.g. reportNode)? In my opinion, repeating the same report message four times in the code is a bit undesirable.

return;
}

if (checkObjects && !isArrayIndexAccess(rightNode)) {
const property = rightNode.property;

if (requireRenaming && !rightNode.computed) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the !rightNode.computed check is necessary here; computed properties aren't any different from other properties in this case:

const foo = bar[baz];

// can be replaced with

const {[baz]: foo} = bar;

Copy link
Contributor Author

@alexlafroscia alexlafroscia Dec 13, 2016

Choose a reason for hiding this comment

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

Oh wow, I had no idea that that syntax was valid. That is.... pretty weird. But, you're right! I'll add some test cases around that syntax and update the rule.

context.report({ node: reportNode, message: "Use object destructuring" });
return;
}

if (property.type === "Literal" && leftNode.name === property.value) {
context.report({ node: reportNode, message: "Use object destructuring" });
}

if (property.type === "Identifier" && leftNode.name === property.name) {
context.report({ node: reportNode, message: "Use object destructuring" });
}
}
}

/**
* Check if a given variable declarator is coming from an property access
* that should be using destructuring instead
*
* @param {ASTNode} node the variable declarator to check
* @returns {void}
*/
function checkVariableDeclarator(node) {

// Skip if variable is declared without assignment
if (!node.init) {
return;
}

// We only care about member expressions past this point
if (node.init.type !== "MemberExpression") {
return;
}

performCheck(node.id, node.init, node);
}

/**
* Run the `prefer-destructuring` check on an AssignmentExpression
*
* @param {ASTNode} node the AssignmentExpression node
* @returns {void}
*/
function checkAssigmentExpression(node) {
performCheck(node.left, node.right, node);
}

//--------------------------------------------------------------------------
// Public
//--------------------------------------------------------------------------

return {
VariableDeclarator: checkVariableDeclarator,
AssignmentExpression: checkAssigmentExpression
};
}
};
122 changes: 122 additions & 0 deletions tests/lib/rules/prefer-destructuring.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/**
* @fileoverview Prefer destructuring from arrays and objects
* @author Alex LaFroscia
*/
"use strict";

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

const rule = require("../../../lib/rules/prefer-destructuring"),
RuleTester = require("../../../lib/testers/rule-tester");


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

const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });

ruleTester.run("prefer-destructuring", rule, {
valid: [
{
code: "var [foo] = array;"
},
{
code: "var { foo } = object;"
},
{

// Ensure that this doesn't break variable declarating without assignment
code: "var foo;"
},
{
code: "var foo = object.bar;",
options: [{ object: true }, { requireRenaming: false }]
},
{
code: "var foo = object['bar'];",
options: [{ object: true }, { requireRenaming: false }]
},
{
code: "var { bar: foo } = object;",
options: [{ object: true }, { requireRenaming: true }]
},
{
code: "var foo = object[bar];",
options: [{ object: true }, { requireRenaming: true }]
},
{
code: "var foo = array[0];",
options: [{ array: false }]
},
{
code: "var foo = object.foo;",
options: [{ object: false }]
},
{
code: "var foo = object['foo'];",
options: [{ object: false }]
},
{
code: "({ foo } = object);"
},
{
code: "[foo] = array;"
}
],

invalid: [
{
code: "var foo = array[0];",
errors: [{
message: "Use array destructuring",
type: "VariableDeclarator"
}]
},
{
code: "foo = array[0];",
errors: [{
message: "Use array destructuring",
type: "AssignmentExpression"
}]
},
{
code: "var foo = array.foo;",
errors: [{
message: "Use object destructuring",
type: "VariableDeclarator"
}]
},
{
code: "var foo = array.bar;",
options: [{ object: true }, { requireRenaming: true }],
errors: [{
message: "Use object destructuring",
type: "VariableDeclarator"
}]
},
{
code: "var foo = array['foo'];",
errors: [{
message: "Use object destructuring",
type: "VariableDeclarator"
}]
},
{
code: "foo = array.foo;",
errors: [{
message: "Use object destructuring",
type: "AssignmentExpression"
}]
},
{
code: "foo = array['foo'];",
errors: [{
message: "Use object destructuring",
type: "AssignmentExpression"
}]
}
]
});