Add consistent-destructuring rule

[medusalix]

Resolves #248

---

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

• #248: Rule proposal: Consistent destructuring

---

[medusalix]

@MrHen I added all of your suggested test cases, though I'm still not sure how to tackle this one:

let foo = {a: 1};
const {a} = foo;
foo = {a: 2};
console.log(foo.a);

I found another example in this codebase:

const spinner = new Ora('foo');
spinner.start();
const {id} = spinner;
spinner.start();
t.is(id, spinner.id);

The spinner.start() function somehow changes the spinner.id property. My fixer would change the last statement to this:

t.is(id, id);

Now it doesn't make sense anymore.

[MrHen]

I'm still not sure how to tackle this one

There needs to be a check to see if a variable in that scope already exists. If there is one, either (a) use avoid-capture to pick a name not being used or (b) chose not to fix it at all.

I lean toward (b) because this is probably an unusual situation.

[medusalix]

I'm not talking about the case where the variable is already defined and therefore can't be destructured. I'm concerned about the situtation where the destructured member is changed after being destructured (The last test case you suggested). If I apply my fix to this scenario, the semantics of the code will change.

[MrHen]

I'm concerned about the situtation where the destructured member is changed after being destructured.

Oh, I see. I wasn't even originally worried about that but now I get the issue. A really common example of this would be getters:

class Foo {
  let _a = 'foo';

  set a(value) {
    this._a = value;
  }

  get a() {
    return this._a;
  }
}

const foo = new Foo();
const { a } = foo;
foo.a = 'bar';
console.log(a, foo.a);

The smallest example would just be:

const foo = {a: 1};
const {a} = foo;
foo.a = 2;
console.log(a, foo.a);

I'm not sure how to deal with this, either. @sindresorhus, @futpib, either of you have any thoughts / insights?

[sindresorhus]

The prefer-destructuring rule has a fixer, so not sure how it handles it. @medusalix Can you take a look at its source and tests and see whether it handles this or just ignores it?

[medusalix]

@sindresorhus The prefer-destructuring rule is apparently only tailored to the case of variable assignments. All of the failing tests I've tried passed that rule without any warning at all.

[sindresorhus]

I think for that case we can use the ESLint suggestions API instead of a fixer. Then the user can decide to manually apply the suggestion.

[medusalix]

I have now switched to the suggestions API as suggested and it seems to be working great. It catched a bunch of problems in all kinds of different rules, that's why the lint task is failing.

[fisker]

@medusalix Don't try to fix lint, We can do review first.

[fisker]

Did you merge the latest master? Failed integration test should has a link link to the source code.

[fisker]

Integration tests need to be fixed, you can use this as an example, find the code add to tests.

[medusalix]

Destructurings of function calls inside member expression should be ignored now. Might need some additional tests.

[sindresorhus]

Might need some additional tests.

Can you add some?

[sindresorhus]

Friendly bump :)

[medusalix]

I've added tests for various kinds of expressions (as defined by the ESTree specification) and discovered a bug in the VariableDeclarator handler of the rule. I've decided to add a check to ignore complex expressions (anything that's not a MemberExpression or Identifier) which fixed those problems.