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

Merged
merged 8 commits into from Dec 16, 2016

Projects

None yet

9 participants

@alexlafroscia
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[x] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Please describe what the rule should do:

This rule ensures that all object property access and array index accesses are done through ES6 destructuring.

What category of rule is this? (place an "X" next to just one item)

[ ] Enforces code style
[ ] Warns about a potential error
[x] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

- var foo = array[0];
+ var [ foo ] = array;

- var foo = object.foo;
+ var { foo } = object;

- var foo = object['foo'];
+ var { foo } = object;

Why should this rule be included in ESLint (instead of a plugin)?

This change supports (an encourages) a new JavaScript feature that isn't tied directly to any particular framework or library. Additionally, it is part of an existing plugin, eslint-plugin-ember-suave, where I originally authored the rule. However, someone requested that the rule be added to core in #6053 and asked if I would submit it to core.

What changes did you make? (Give an overview)

  • Added a new rule, prefer-destructuring, as well as the tests and documentation that I originally authored as part of the eslint-plugin-ember-suave.

Is there anything you'd like reviewers to focus on?

Please make sure the tests and documentation are thorough enough. I just copied the implementation, tests and docs from the existing plugin (linked above), which may not have the same level of standards that the core rules have, although I did my best to follow all the best practices when I originally wrote the rule.

@alexlafroscia alexlafroscia New: `prefer-destructuring` rule (fixes #6053)
7b0b3db
@jsf-clabot
jsf-clabot commented Dec 11, 2016 edited

CLA assistant check
All committers have signed the CLA.

@mention-bot

@alexlafroscia, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @alberto and @kaicataldo to be potential reviewers.

@eslintbot

LGTM

@not-an-aardvark

Thanks for the pull request! I made a few suggestions.

docs/rules/prefer-destructuring.md
+
+## Rule Details
+
+The following patterns are considered warnings:
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

In order to get the correct red/green highlight colors in the docs (example), I think the text above the code samples needs to have very specific formatting:

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

Examples of **correct** code for this rule:
docs/rules/prefer-destructuring.md
+```json
+{
+ "rules": {
+ "ember-suave/prefer-destructuring": ["error", {
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

This should just be prefer-destructuring.

@alexlafroscia
alexlafroscia Dec 11, 2016 Contributor

Oops, thanks!

lib/rules/prefer-destructuring.js
+ type: "boolean"
+ }
+ }
+ }
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

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}].

lib/rules/prefer-destructuring.js
+ checkArrays = options.array;
+ }
+
+ if (options.hasOwnProperty("object")) {
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

Nit: Can you use one of the following instead?

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

// or alternatively

if (Object.prototype.hasOwnProperty.call(options, "object")) {
    checkObjects = options.object;
}

With options.hasOwnProperty, the rule will throw an unexpected error if the user passes {hasOwnProperty: 5} as the options object. (Admittedly I'm not sure why a user would want to do this, but I think it's still better to avoid that possibility.)

+ * @returns {boolean} whether or not the node is an integer
+ */
+ function isArrayIndexAccess(node) {
+ return Number.isInteger(node.property.value);
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

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;
@alexlafroscia
alexlafroscia Dec 11, 2016 Contributor

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.

@alexlafroscia
alexlafroscia Dec 11, 2016 Contributor

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.

lib/rules/prefer-destructuring.js
+ }
+
+ if (node.init.type === "Identifier") {
+ const objectExpression = node.id.type === "ObjectPattern" && node.id;
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

It's not a big issue, but this line is a bit confusing to me (at first glance, the && node.id part seemed redundant). I see what the code is trying to do, but I think it would be clearer if it looked something like this instead:

if (node.id.type === "ObjectPattern") {
    const objectExpression = node.id;
    // ...
}
lib/rules/prefer-destructuring.js
+ const objectExpression = node.id.type === "ObjectPattern" && node.id;
+
+ if (objectExpression) {
+ const prop = objectExpression.properties[0];
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

As far as I can tell, this is only checking the first property of the object pattern. I'm not sure we should do this check at all in this rule (see my comment on prefer-destructuring below), but if we do decide to keep the check in this rule, it would be better if the check was performed for all properties, not just the first property.

lib/rules/prefer-destructuring.js
+ const key = prop.key;
+ const value = prop.value;
+
+ if ((key.name === value.name) && (key.start !== value.start)) {
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

It would be better to use if (!prop.shorthand) instead of if (key.start !== value.start).

lib/rules/prefer-destructuring.js
+ const value = prop.value;
+
+ if ((key.name === value.name) && (key.start !== value.start)) {
+ context.report({ node: prop, message: "Unnecessary duplicate variable name" });
@not-an-aardvark
not-an-aardvark Dec 11, 2016 edited Member

From what I can tell, this logic is intended to report code like the following:

var {foo: foo} = bar;

But I don't think this check should be part of the prefer-destructuring rule, because we already have the object-shorthand rule to handle code like this. (edit: Actually it's the no-useless-rename rule, but it doesn't change this point.)

@alexlafroscia
alexlafroscia Dec 11, 2016 Contributor

Yes, you're right that that's what that intends to protect against. Assuming object-shorthand would catch that, I can remove the check here.

lib/rules/prefer-destructuring.js
+ const variableIdentifier = node.id;
+ const property = node.init.property;
+
+ if (property.type === "Literal" && variableIdentifier.name === property.value) {
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

It looks like this only reports

var foo = bar.foo;

but it doesn't report

var foo = bar.baz;

I think this is incorrect behavior, because the latter could be replaced with

var {baz: foo} = bar;
@alexlafroscia
alexlafroscia Dec 11, 2016 Contributor

Good point. I think it would be good to solicit some feedback on this. My assumption was that, if the property name and variable name didn't match, the rule just isn't applicable. However, we're right that it could be done the other way, although writing out

var { baz: foo } = bar;

is way more confusing to read than

var foo = bar.baz;

I'm not sure the rule should enforce the first destructure-and-alias-name version; maybe that should be a configuration option?

@platinumazure
platinumazure Dec 11, 2016 Member

👍 on having that at least be a configuration option-- the destructuring way seems a bit confusing.

@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

I'd also be fine with having a configuration option for it.

@alexlafroscia
alexlafroscia Dec 11, 2016 Contributor

So, that would make the configuration options look something like:

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

to exhibit the current behavior (not requiring that mis-matched names be destructured) and then something like

{
  "rules": {
    "prefer-destructuring": ["error", {
      "array": true,
      "object": { "some-configuration-key": true }
    }]
  }
}

for the more strict approach? What might be a good name for some-configuration-key?

@alexlafroscia
alexlafroscia Dec 11, 2016 Contributor

Or, would the more-strict definition be the default, with an option to make it more relaxed?

@alexlafroscia
alexlafroscia Dec 12, 2016 edited Contributor

The refactoring I did here makes it seem like this was addressed (because the code changed), but it hasn't been yet.

It's too bad there's no way to tell GitHub that this chain of comments isn't actually outdated.

@not-an-aardvark
not-an-aardvark Dec 12, 2016 Member

I had been thinking it would be a separate option, along the lines of

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

But I'm definitely open to bikeshedding on the option name/configuration.


In general, I slightly prefer that rules have strict behavior by default, and allow the user to manually relax the behavior. My reasoning is that a user might want to just turn on the rule before carefully considering the available options. If the rule is strict by default and the user preferred the loose setting, the issue will probably be obvious (errors will get reported, and the user will look up the options and relax the behavior). On the other hand, if the rule is loose by default and the user preferred the strict setting, the user might not notice anything is wrong, and they'll think ESLint is validating something when it's actually not.

@alexlafroscia
alexlafroscia Dec 12, 2016 Contributor

RE: strict by default, I think that sounds like a great justification for going that route.

I'll take a look at implementing that more-strict behavior as optional ASAP, based on the configuration that you demonstrated. If someone proposes a syntax that we like better, that should be easy enough to change.

@alexlafroscia
alexlafroscia Dec 13, 2016 Contributor

Add the configuration suggested above and some tests to cover the it.

lib/rules/prefer-destructuring.js
+ //--------------------------------------------------------------------------
+
+ return {
+ VariableDeclarator: checkVariableDeclarator
@not-an-aardvark
not-an-aardvark Dec 11, 2016 Member

It looks like this only checks VariableDeclarator nodes, but I think it should also check AssignmentExpression nodes.

For example, the following should get reported as an error:

foo = bar[0];

Since it could get replaced with:

[foo] = bar;

This should also get reported:

foo = bar.baz;

because it could be replaced with:

({baz: foo} = bar);
@alexlafroscia
alexlafroscia Dec 11, 2016 Contributor

Good catches! I'll add some tests around those cases and improve the rule.

tests/lib/rules/prefer-destructuring.js
+// Tests
+//------------------------------------------------------------------------------
+
+const ruleTester = new RuleTester();
@not-an-aardvark
not-an-aardvark Dec 11, 2016 edited Member

Nit: Instead of repeating { parserOptions: { ecmaVersion: 6 } } for each test, it would be better to pass this as a default to the RuleTester constructor.

const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
alexlafroscia added some commits Dec 11, 2016
@alexlafroscia alexlafroscia Address general, easy PR feedback 445e230
@alexlafroscia alexlafroscia Remove checks covered by `object-shorthand`
4252bf1
@eslintbot

LGTM

@alexlafroscia alexlafroscia Add support for AssignmentExpression in `prefer-destructuring`
cc5f7dc
@eslintbot

LGTM

@alexlafroscia alexlafroscia Add `requireRenaming` option to the `prefer-destructuring` rule
33e23bd
@eslintbot

LGTM

@not-an-aardvark

This mostly looks good to me, I just have a few nitpicks.

lib/rules/prefer-destructuring.js
+ }
+
+ if (checkArrays && isArrayIndexAccess(rightNode)) {
+ context.report({ node: reportNode, message: "Use array destructuring" });
@not-an-aardvark
not-an-aardvark Dec 13, 2016 Member

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.

lib/rules/prefer-destructuring.js
+ if (checkObjects && !isArrayIndexAccess(rightNode)) {
+ const property = rightNode.property;
+
+ if (requireRenaming && !rightNode.computed) {
@not-an-aardvark
not-an-aardvark Dec 13, 2016 Member

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;
@alexlafroscia
alexlafroscia Dec 13, 2016 edited Contributor

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.

docs/rules/prefer-destructuring.md
+
+### 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:
@not-an-aardvark
not-an-aardvark Dec 13, 2016 Member

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).

@not-an-aardvark
Member

I'll repost the discussion here for greater visibility, since the original discussion is hidden under an "outdated diff" thing.

Basically, there is an option to toggle whether or not the rule should require destructuring in cases where properties would need to be renamed.

const foo = bar.baz;

// this could be replaced with 
const {baz: foo} = bar;

We should decide:

  • What should the option be called? (Currently, it's called requireRenaming)
  • What should the rule do by default? (Currently, the rule does require renaming by default. I argued here for the strict-by-default behavior, but I'm starting to reconsider this because the second line above seems significantly less readable than the first line.)
@kaicataldo
Member
kaicataldo commented Dec 13, 2016 edited

What should the option be called?

I think we should avoid require in this option name since it's kind of a loaded term (not a big deal, but I think it could be clearer to use something like prefer or enforce).

What should the rule do by default?

My preference would be to leave the option off (should not enforce destructuring with renaming), because I don't think that's a common pattern and agree that it's harder to read than the first.

@ljharb
Contributor
ljharb commented Dec 13, 2016

How about enforceForRenamedProperties? (definitely defaulted to off)

@alexlafroscia alexlafroscia More PR feedback
8fc52e6
@eslintbot

LGTM

@alexlafroscia
Contributor

Alright, I updated the docs, renamed the additional configuration option to enforceForRenamedProperties, made it off by default and required that crazy

var { [bar]: foo } = object;

syntax if it is enabled.

docs/rules/prefer-destructuring.md
@@ -23,18 +23,39 @@ var [ foo ] = array;
// With `object` enabled
var { foo } = object;
+var foo = object.bar
@ljharb
ljharb Dec 14, 2016 Contributor

;?

@alexlafroscia
alexlafroscia Dec 14, 2016 Contributor

Great catch, thanks!

tests/lib/rules/prefer-destructuring.js
errors: [{
message: "Use object destructuring",
type: "VariableDeclarator"
}]
},
{
- code: "var foo = array.bar;",
- options: [{ object: true }, { requireRenaming: true }],
+ code: "var foo = object[bar];",
@ljharb
ljharb Dec 14, 2016 Contributor

i'm not sure i understand - object[bar] isn't the same as object.bar. How would you use destructuring here? Does var { [bar]: foo } = object; work?

@not-an-aardvark
not-an-aardvark Dec 14, 2016 edited Member

Apparently it does, yes. ¯\_(ツ)_/¯

@ljharb
ljharb Dec 14, 2016 Contributor

how do i javascript, 2016 edition

@not-an-aardvark
not-an-aardvark Dec 14, 2016 Member

For bonus Readable Code™ points, figure out what this function does!

(({[foo = 1]:[foo = 2]} = {[foo = 3]:[foo = 4]}) => ({[foo = 5]:[foo = 6]}))
@ljharb
ljharb Dec 14, 2016 Contributor

const z̲̗̼͙̥͚͛͑̏a̦̟̳͋̄̅ͬ̌͒͟ļ̟̉͌ͪ͌̃̚g͔͇̯̜ͬ̒́o̢̹ͧͥͪͬ =

@kaicataldo
kaicataldo Dec 14, 2016 Member

Huh...TIL

@not-an-aardvark

LGTM. Thanks for all the work you put into this!

@alexlafroscia alexlafroscia Add missing semicolon to code example in docs
c81fcb2
@eslintbot

LGTM

@ilyavolodin

Just couple of small notes for documentation.

docs/rules/prefer-destructuring.md
@@ -0,0 +1,82 @@
+# 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.
@ilyavolodin
ilyavolodin Dec 14, 2016 Member

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

@alexlafroscia
alexlafroscia Dec 15, 2016 Contributor

Yup, thanks!

docs/rules/prefer-destructuring.md
+
+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.
+
+## Rule Details
@ilyavolodin
ilyavolodin Dec 14, 2016 Member

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];

@alexlafroscia
alexlafroscia Dec 15, 2016 Contributor

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

// With `array` enabled
var [ foo ] = array;
var foo = array[someIndex];
@ilyavolodin
ilyavolodin Dec 15, 2016 edited Member

That sounds good too.

@alexlafroscia alexlafroscia referenced this pull request in DockYard/eslint-plugin-ember-suave Dec 15, 2016
Closed

Feature/document rule overrides in readme #39

@alexlafroscia alexlafroscia Update documentation
fc63f7f
@eslintbot

LGTM

@ilyavolodin

LGTM. Thanks

@not-an-aardvark not-an-aardvark merged commit 27424cb into eslint:master Dec 16, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@not-an-aardvark
Member

Thanks for contributing to ESLint!

@kaicataldo
Member

@alexlafroscia Appreciate you being so willing to add this when we asked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment