Update: support more options in prefer-destructuring #8796

VictorHom · 2017-06-24T21:55:31Z

[X ] Changes an existing rule (template)

What rule do you want to change?
prefer-destructuring.js
This pr is in reference to the issue #7886

Does this change cause the rule to produce more or fewer warnings?
less warnings since you have more flexibility to control destructuring errors.

How will the change be implemented? (New option, new default behavior, etc.)?
The first parameter of destructuring has been updated to also take a VariableDeclarator value or an AssignmentExpression value. Each value takes an object that takes an object of optional keys array and/or object.

The old parameters of {object: Boolean, array: Boolean} is still supported

Please provide some example code that this change will affect:

{
  "rules": {
    "prefer-destructuring": ["error", {
      "VariableDeclaration": {
        "array": false,
        "object": true
      },
     "AssignmentExpression": {
        "array": true,
        "object": true
      }
    }, {
      "enforceForRenamedProperties": false
    }]
  }
}

What does the rule currently do for this code?
In the above example, prefer-destructuring will be enforced for variable declarations from objects but not from arrays. Prefer-destructuring will be enforced for assignment expressions for both arrays and objects

What will the rule do after it's changed?
It will let users have more flexibility and control over when prefer-destructuring should be enforced.

eslintbot · 2017-06-24T21:55:32Z

LGTM

mention-bot · 2017-06-24T21:55:32Z

@VictorHom, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexlafroscia, @not-an-aardvark and @flowmemo to be potential reviewers.

eslintbot · 2017-06-24T21:58:01Z

LGTM

eslintbot · 2017-06-24T22:02:30Z

LGTM

not-an-aardvark

Thanks for the PR! I left a few comments.

not-an-aardvark · 2017-06-25T01:03:52Z

docs/rules/prefer-destructuring.md

@@ -33,7 +33,9 @@ This rule takes two sets of configuration objects; the first controls the types

 The first has 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`.

-The second has a single property, `enforceForRenamedProperties`, that controls whether or not the `object` destructuring rules are applied in cases where the variable requires the property being access to be renamed.
+The first has two properties, `VariableDeclarator` and `AssignmentExpression`, which can be used to turn on or off the destructuring requirement for each of those types independently. Each property accepts an object that accpets two properties, `array` and `object`, which can be used to turn on or off the destructuring requirement for each of those types independently for variable declarations and assignment expressions.  By default, `array` and `object` are set to true for both `VariableDeclarator` and `AssignmentExpression`.


Sorry if this was unclear before, but I think we should continue to allow the array and object top-level options. Making breaking changes to rule schemas is generally a pretty big deal -- in addition to breaking users, it also tends to make shareable configs invalid, which makes upgrading very tedious -- so we try to almost never do it. (Changing the default behavior of a rule schema while keeping all previous schemas valid is a bit better, but that's a separate topic.)

To avoid extra complexity, we can disallow mixing the old and new behaviors -- for example, this would still be an illegal config:

rules: prefer-destructuring: [error, {array: true, VariableDeclarator: {array: false}}]

not-an-aardvark · 2017-06-25T01:05:08Z

docs/rules/prefer-destructuring.md

-The second has a single property, `enforceForRenamedProperties`, that controls whether or not the `object` destructuring rules are applied in cases where the variable requires the property being access to be renamed.
+The first has two properties, `VariableDeclarator` and `AssignmentExpression`, which can be used to turn on or off the destructuring requirement for each of those types independently. Each property accepts an object that accpets two properties, `array` and `object`, which can be used to turn on or off the destructuring requirement for each of those types independently for variable declarations and assignment expressions.  By default, `array` and `object` are set to true for both `VariableDeclarator` and `AssignmentExpression`.
+
+The second has a single property, `enforceForRenamedProperties`, that controls whether or not the `object` destructuring rules are applied in cases where the variable requires the property being access to be renamed. If `object` in `AssignmentExpression` is set to false, `enforceForRenamedProperties` property is ignored.


Why is enforceForRenamedProperties ignored in this case? It seems like it would still be useful in variable declarations for something like

var foo = bar.baz // -> var {baz: foo} = bar;

not-an-aardvark · 2017-06-25T01:09:24Z

lib/rules/prefer-destructuring.js

                report(reportNode, "object");
                return;
            }

-            if (checkObjects) {
+            if ((checkObjectsInVariableDeclarator && reportNode.type === "VariableDeclarator") || (checkObjectsInAssignmentExpression && reportNode.type === "AssignmentExpression")) {


I think the value in this if statement is redundant, because reportNode.type will always be either "VariableDeclarator" or "AssignmentExpression". It can be simplified to

if (checkObjectsInVariableDeclarator || checkObjectsInAssignmentExpression) { // ... }

not-an-aardvark · 2017-06-25T01:18:07Z

lib/rules/prefer-destructuring.js

+                const assignmentExpression = enabledTypes.AssignmentExpression;
+
+                checkArraysInAssignmentExpression = assignmentExpression.hasOwnProperty("array") ? assignmentExpression.array : checkArraysInAssignmentExpression;
+                checkObjectsInAssignmentExpression = assignmentExpression.hasOwnProperty("object") ? assignmentExpression.object : checkObjectsInAssignmentExpression;
            }
        }


This config-checking logic is getting kind of complicated. I think having a variable for each config case won't scale very well as more options are added, especially if we restore support for the old top-level "array" option. Can we extract this into a function instead? For example:

/** * @param {string} nodeType "AssignmentExpression" or "VariableDeclaration" * @param {string} destructuringType "array" or "object" * @returns {boolean} `true` if the destructuring type should be checked for the given node */ function shouldCheck(nodeType, destructuringType) { return context.options[0] && context.options[0][nodeType] && context.options[0][nodeType][destructuringType] === false; }

Then rather than handling four different variables, the performCheck function, it could just use something like

if (isArrayIndexAccess(node) && shouldCheck(reportNode.type, "array")) { // ... } // ...

eslintbot · 2017-06-25T13:26:50Z

LGTM

eslintbot · 2017-06-25T16:02:38Z

LGTM

kaicataldo

Thanks for contributing! I have a few questions.

Also, I know this is from the proposal, but I wonder if VariableDeclaration might make more sense as an option instead of VariableDeclarator. @not-an-aardvark I know you proposed this originally - what do you think?

kaicataldo · 2017-06-30T04:22:23Z

docs/rules/prefer-destructuring.md

+{
+  "rules": {
+    "prefer-destructuring": ["error", {
+      "VariableDeclaration": {


Should this be VariableDeclarator to match the other uses? I actually think VariableDeclaration might make more sense.

just want to make sure - you mean that you would prefer the attribute to be named VariableDeclaration?

I feel like it makes more sense to someone who's not familiar with ESTree and node types, but let's see what others say. I don't want to hold this up over a naming thing, but wanted to bring it up since it is more difficult to change things once they're merged!

I like VariableDeclarator because I feel it's a closer parallel to AssignmentExpression, but I can see the potential for confusion. No strong opinion from me, though.

Let's leave it as VariableDeclarator - I don't want to hold this PR up over a naming thing, especially since I was late to the game in the disxussion. Sorry @VictorHom, looks like you already changed it

totally fine! I'm sure there's a few more things that could use some cleaning up too.

kaicataldo · 2017-06-30T04:23:11Z

docs/rules/prefer-destructuring.md

@@ -62,6 +62,45 @@ An example configuration, with the defaults filled in, looks like this:
 }
 ```

+Alternatively, you can use a different set of configuration objects to have more control on what to enforce destructuring. It also takes two sets of configuration objects.
+
+The first has two properties, `VariableDeclarator` and `AssignmentExpression`, which can be used to turn on or off the destructuring requirement for each of those types independently. Each property accepts an object that accpets two properties, `array` and `object`, which can be used to turn on or off the destructuring requirement for each of those types independently for variable declarations and assignment expressions.  By default, `array` and `object` are set to true for both `VariableDeclarator` and `AssignmentExpression`.


Typo: accpets -> accepts

kaicataldo · 2017-06-30T04:29:50Z

lib/rules/prefer-destructuring.js

@@ -110,22 +167,21 @@ module.exports = {
            }

            if (isArrayIndexAccess(rightNode)) {
-                if (checkArrays) {
+                if (shouldCheck("VariableDeclarator", "array") || shouldCheck("AssignmentExpression", "array") || checkArrays) {


Apologies if I'm misunderstanding something here, but I'm curious what happens for all these checks when the node being evaluated is either a VariableDeclarator or AssignmentExpression and the configuration is set to true for the other node type.

Some example test cases to illustrate what I mean:

{ code: "foo = object.foo;", options: [{ VariableDeclarator: { object: true }, AssignmentExpression: { object: false } }] }, { code: "var foo = object.foo;", options: [{ VariableDeclarator: { object: false }, AssignmentExpression: { object: true } }] }

Good point. With this configuration, both of these are valid cases, along with a few more cases that I added

eslintbot · 2017-07-01T15:19:58Z

LGTM

eslintbot · 2017-07-01T17:12:44Z

LGTM

not-an-aardvark

Sorry about the delay. This looks mostly good, I just have a few small suggestions.

not-an-aardvark · 2017-07-05T07:47:11Z

docs/rules/prefer-destructuring.md


-The second has a single property, `enforceForRenamedProperties`, that controls whether or not the `object` destructuring rules are applied in cases where the variable requires the property being access to be renamed.
+The second has a single property, enforceForRenamedProperties, that controls whether or not the object destructuring rules are applied in cases where the variable requires the property being access to be renamed.


Can you surround enforceForRenamedProperties in backticks?

not-an-aardvark · 2017-07-05T07:53:36Z

docs/rules/prefer-destructuring.md

+
+The first has two properties, `VariableDeclarator` and `AssignmentExpression`, which can be used to turn on or off the destructuring requirement for each of those types independently. Each property accepts an object that accepts two properties, `array` and `object`, which can be used to turn on or off the destructuring requirement for each of those types independently for variable declarations and assignment expressions.  By default, `array` and `object` are set to true for both `VariableDeclarator` and `AssignmentExpression`.
+
+The second has a single property, `enforceForRenamedProperties`, that controls whether or not the `object` destructuring rules are applied in cases where the variable requires the property being access to be renamed.


I think this documentation can be simplified because the enforceForRenamedProperties option is the same in either case. It seems a bit confusing that the description of that option is duplicated.

I'm picturing the documentation looking like this, but feel free to organize it differently:

This rule accepts an object as the first option, which determines what types of destructuring the rule applies to. (...more description about array and object options)
Alternatively, you can use separate configurations for different assignment types. (...more description about VariableDeclarator and AssignmentExpression options)
The rule also has a second object option with a single key ,enforceForRenamedProperties, which determines whether the object destructuring rule applies to renamed variables.

not-an-aardvark · 2017-07-05T07:55:34Z

lib/rules/prefer-destructuring.js

+                                properties: {
+                                    array: {
+                                        type: "boolean",
+                                        default: true


Does the default property have any effect? I hadn't been aware it exists 😃

I like it for the clarity, but it has no visible effect from running tests.
I used after checking the examples in https://github.com/json-schema-org/json-schema-org.github.io/search?utf8=%E2%9C%93&q=default%3A&type=
let me know if that’s okay. otherwise, I’m not tied to it and if removing makes more sense, then I can remove.

not-an-aardvark · 2017-07-05T08:01:26Z

lib/rules/prefer-destructuring.js

+            if (typeof enabledTypes.VariableDeclarator !== "undefined" || typeof enabledTypes.AssignmentExpression !== "undefined") {
+                checkArrays = false;
+                checkObjects = false;
+            }


It's not a blocker for this PR, but having two separate methods of checking the options depending on the type of configuration used seems like a bug hazard if something changes later on. Maybe the old config-checking variables could be eliminated too with something like

const normalizedOptions = typeof enabledTypes.array !== "undefined" || typeof context.options[0].object !== "undefined" ? { VariableDeclarator: enabledTypes, AssignmentExpression: enabledTypes } : enabledTypes

In other words, the old array and object schema would basically be transformed into a special case of the VariableDeclarator/AssignmentExpression schema where both assignment types happen to have the same configured options.

updated to using a normalizedOptions

not-an-aardvark · 2017-07-05T08:02:19Z

lib/rules/prefer-destructuring.js

@@ -110,22 +167,27 @@ module.exports = {
            }

            if (isArrayIndexAccess(rightNode)) {
-                if (checkArrays) {
+                if ((shouldCheck("VariableDeclarator", "array") && reportNode.type === "VariableDeclarator") ||
+                    (shouldCheck("AssignmentExpression", "array") && reportNode.type === "AssignmentExpression") ||


I think this could be simplified by just using if (shouldCheck(reportNode.type, "array")).

The main reason why I added the reportNode.type check is to handle cases

{ code: "foo = array[0];", options: [{ VariableDeclarator: { array: true }, AssignmentExpression: { array: false } }, { enforceForRenamedProperties: false }] }, { code: "var foo = array[0];", options: [{ VariableDeclarator: { array: false }, AssignmentExpression: { array: true } }, { enforceForRenamedProperties: false }] },

where the check for array/object is swapped between variabledeclarator and assignmentexpression

not-an-aardvark · 2017-07-05T08:04:03Z

tests/lib/rules/prefer-destructuring.js

        },
        {
-            code: "var foo = object[bar];",
+            code: "var foo = object['bar'];",


Is this change necessary? (I don't mind the new version, I'm just sometimes worried about changing old tests because that can prevent regressions from being caught.)

changed back. not sure why I updated to add quotes.

not-an-aardvark · 2017-07-05T08:07:32Z

tests/lib/rules/prefer-destructuring.js

+            }]
+        },
+        {
+            code: "var foo = object[bar];",


Can you add some additional invalid tests? It would be nice to see:

A test for the array option with the VariableDeclarator/AssignmentExpression option style

A test with the AssignmentExpression option

A test where the VariableDeclarator and AssignmentExpression options are different (e.g. if the code has an object property access without destructuring in an assignment expression, and the options are {AssignmentExpression: {object: true}, VariableDeclarator: {object: false}}

Added more invalid tests

eslintbot · 2017-07-11T03:54:30Z

LGTM

eslintbot · 2017-07-11T13:21:37Z

LGTM

VictorHom · 2017-07-13T02:37:56Z

@not-an-aardvark just want to bring this pr back up for review when you have some time

not-an-aardvark

Thanks, I left a few more comments.

not-an-aardvark · 2017-07-13T03:07:26Z

docs/rules/prefer-destructuring.md

@@ -29,11 +29,15 @@ var foo = object.bar;

 ### Options

-This rule takes two sets of configuration objects; the first controls the types that the rule is applied to, and the second controls the way those objects are evaluated.


Can you move this section to before the code samples?

not-an-aardvark · 2017-07-13T03:11:03Z

lib/rules/prefer-destructuring.js

        const enabledTypes = context.options[0];
        const additionalOptions = context.options[1];
+        let enforceForRenamedProperties = false;


Nitpick: I think this can just be let enforceForRenamedProperties = context.options[1] && context.options[1].enforceForRenamedProperties;

not-an-aardvark · 2017-07-13T03:25:44Z

lib/rules/prefer-destructuring.js

@@ -110,22 +159,26 @@ module.exports = {
            }

            if (isArrayIndexAccess(rightNode)) {
-                if (checkArrays) {
+                if ((shouldCheck("VariableDeclarator", "array") && reportNode.type === "VariableDeclarator") ||
+                    (shouldCheck("AssignmentExpression", "array") && reportNode.type === "AssignmentExpression")) {


(Continuing the discussion from #8796 (comment))

I'm not sure I understand -- wouldn't that result in the same behavior? Assuming reportNode.type is always either VariableDeclarator or AssignmentExpression, it seems like shouldCheck(reportNode.type, "array") would result in the same behavior. (The if statement here has two cases, but only one of them is relevant at any given time, so shouldCheck(reportNode.type, "array") would just combine the two cases.)

I see what you are saying now. I misinterpreted. sorry for the confusion.

not-an-aardvark · 2017-07-13T03:26:49Z

lib/rules/prefer-destructuring.js

+                                properties: {
+                                    array: {
+                                        type: "boolean",
+                                        default: true


(Continuing the discussion from #8796 (comment))

If it has no effects, then I think we should remove it, because it could be misleading and make people think that it does have an effect. If we want to indicate the default there, I think adding a comment would be better.

Generally, we've let the documentation and code speak for itself in this area

I will remove in that case

not-an-aardvark · 2017-07-13T03:28:04Z

docs/rules/prefer-destructuring.md


-The first has 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`.
+The two properties, `array` and `object`, can be used to turn on or off the destructuring requirement for each of those types independently. By default, both are true.


I think it would be useful to give more examples for each option format after these descriptions, e.g.

The 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. For example, the following configuration enforces only object destructuring, but not array destructuring: ```json { "rules": { "prefer-destructuring": ["error", {"object": true, "array": false}] } } ```

not-an-aardvark · 2017-07-13T03:28:26Z

docs/rules/prefer-destructuring.md

-The second has a single property, `enforceForRenamedProperties`, that controls whether or not the `object` destructuring rules are applied in cases where the variable requires the property being access to be renamed.
+Alternatively, you can use separate configurations for different assignment types. It accepts 2 other keys instead of `array` and `object`.
+
+One key is `VariableDeclarator` and the other is `AssignmentExpression`, which can be used to control the destructuring requirement for each of those types independently. Each property accepts an object that accepts two properties, `array` and `object`, which can be used to control the destructuring requirement for each of `array` and `object` independently for variable declarations and assignment expressions.  By default, `array` and `object` are set to true for both `VariableDeclarator` and `AssignmentExpression`.


Can you add some code samples with the VariableDeclarator and AssignmentExpression options?

eslintbot · 2017-07-14T12:01:02Z

LGTM

eslintbot · 2017-07-15T01:04:46Z

LGTM

not-an-aardvark

LGTM, thanks!

I just have one nitpick with the docs, but it shouldn't block this from being merged.

not-an-aardvark · 2017-07-16T04:48:41Z

docs/rules/prefer-destructuring.md

+
+Examples of **correct** code when array destructuring in `AssignmentExpression` is enforced:
+
+```javascript


Can you add a configuration comment to this code block?

/* eslint prefer-destructuring: ["error", {VariableDeclarator: {array: true}}] */

eslintbot · 2017-07-16T12:41:44Z

LGTM

VictorHom · 2017-07-16T13:02:41Z

Awesome! thanks for the code reviews.

@kaicataldo can you also give a review as well?

kaicataldo

LGTM. Thanks for contributing!

not-an-aardvark suggested changes Jun 25, 2017

View reviewed changes

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Jun 25, 2017

VictorHom changed the title ~~Breaking: support more options in prefer-destructuring~~ Update: support more options in prefer-destructuring Jun 25, 2017

VictorHom force-pushed the pr/destructure_reassign_variables branch from 9ac42a3 to 08db36e Compare June 25, 2017 16:02

not-an-aardvark self-requested a review June 28, 2017 01:12

kaicataldo suggested changes Jun 30, 2017

View reviewed changes

not-an-aardvark suggested changes Jul 5, 2017

View reviewed changes

not-an-aardvark suggested changes Jul 13, 2017

View reviewed changes

VictorHom added 2 commits July 14, 2017 21:04

Update: support more options in prefer-destructuring (fixes eslint#7886)

e0cdcc0

fix up

bf14aee

VictorHom force-pushed the pr/destructure_reassign_variables branch from d871981 to bf14aee Compare July 15, 2017 01:04

not-an-aardvark approved these changes Jul 16, 2017

View reviewed changes

add comment to doc

6f68786

kaicataldo approved these changes Jul 21, 2017

View reviewed changes

ilyavolodin merged commit 91dccdf into eslint:master Jul 21, 2017

VictorHom deleted the pr/destructure_reassign_variables branch July 21, 2017 19:26

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018


		The second has a single property, `enforceForRenamedProperties`, that controls whether or not the `object` destructuring rules are applied in cases where the variable requires the property being access to be renamed.
		The second has a single property, enforceForRenamedProperties, that controls whether or not the object destructuring rules are applied in cases where the variable requires the property being access to be renamed.


		The first has two properties, `VariableDeclarator` and `AssignmentExpression`, which can be used to turn on or off the destructuring requirement for each of those types independently. Each property accepts an object that accepts two properties, `array` and `object`, which can be used to turn on or off the destructuring requirement for each of those types independently for variable declarations and assignment expressions. By default, `array` and `object` are set to true for both `VariableDeclarator` and `AssignmentExpression`.

		The second has a single property, `enforceForRenamedProperties`, that controls whether or not the `object` destructuring rules are applied in cases where the variable requires the property being access to be renamed.

		@@ -29,11 +29,15 @@ var foo = object.bar;

		### Options

		This rule takes two sets of configuration objects; the first controls the types that the rule is applied to, and the second controls the way those objects are evaluated.


		The first has 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`.
		The two properties, `array` and `object`, can be used to turn on or off the destructuring requirement for each of those types independently. By default, both are true.


		Examples of correct code when array destructuring in `AssignmentExpression` is enforced:

		```javascript

Update: support more options in prefer-destructuring #8796

Update: support more options in prefer-destructuring #8796

Conversation

VictorHom commented Jun 24, 2017 • edited

eslintbot commented Jun 24, 2017

mention-bot commented Jun 24, 2017

eslintbot commented Jun 24, 2017

eslintbot commented Jun 24, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jun 25, 2017

eslintbot commented Jun 25, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Jul 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jul 1, 2017

eslintbot commented Jul 1, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jul 11, 2017

eslintbot commented Jul 11, 2017

VictorHom commented Jul 13, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jul 14, 2017

eslintbot commented Jul 15, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jul 16, 2017

VictorHom commented Jul 16, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

VictorHom commented Jun 24, 2017 •

edited

kaicataldo Jun 30, 2017 •

edited

kaicataldo Jul 1, 2017 •

edited

kaicataldo Jun 30, 2017 •

edited