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

Do not prefer destructuring for object assignment expression #1583

Merged
merged 1 commit into from Aug 10, 2019

Conversation

@frenzzy
Copy link
Contributor

commented Oct 10, 2017

let param = 'default value';

if (object)
  param = object.param; // <= prefer-destructuring
}

// ...

I think that param = object.param; is better than ({ param } = object); because it is easier to read and tedious to write brackets by hand.

MDN: Destructuring assignment

[eslint config] [base] [patch] Do not prefer destructuring for object…
… assignment expression

```js
let param = 'default value';

if (object)
  param = object.param; // <= prefer-destructuring
}

// ...
```
I think that `param = object.param;` is better than `({ param } = object);` because it is easier to read and tedious to write brackets by hand.

MDN: [Destructuring assignment](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment)
@tunnckoCore
Copy link

left a comment

Good point. 👍

@ljharb
Copy link
Collaborator

left a comment

const { param } = (object || {});?

My concern here is that disabling this setting will make const param = object.param allowed, which it decidedly should not be.

@frenzzy

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2017

One more case:

const fields = _.pick(req.body, allowedFieldNames);

if (!fields.senderEmail) {
  // AssignmentExpression { object: true }
  fields.senderEmail = siteContact.senderEmail; // <= error
  ({ fields.senderEmail } = siteContact); // <= ok
}

if (isValid(fields.recipientEmail)) {
  // AssignmentExpression { array: true }
  fields.recipientEmail = fields.recipientEmail.split(',')[0]; // <= error
  [fields.recipientEmail] = fields.recipientEmail.split(','); // <= ok
}

If you think that destructuring assignments are good, let's just close this.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Oct 11, 2017

@frenzzy to be fair, that example is mutation, which this guide effectively forbids. Try:

const fields = {
  senderEmail: siteContact.senderEmail,
  recipientEmail: siteContact.recipientEmail,
  ...(_.pick(req.body, allowedFieldNames)),
};

Typically when you find awkward code based on our linter rules, it's because the pattern itself is already forbidden or discouraged elsewhere :-)

@DanielGibbsNZ

This comment has been minimized.

Copy link

commented May 20, 2019

Some code that I'm working with uses a similar pattern to the original reporter in quite a few places:

let foo
switch (condition) {
  case SOMETHING.A:
    // other stuff
    foo = object1.foo
    break
  case SOMETHING.b:
    // other stuff
    foo = object2.foo
    break
  case SOMETHING.C:
    // other stuff
    foo = object3.foo
    break
  default:
    // other stuff
    foo = 'fallback value'
}

With the default rule, this forces me to replace the (arguably more readable) foo = object1.foo with ;({ foo } = object1).

@ljharb ljharb self-assigned this May 21, 2019

@ljharb ljharb force-pushed the frenzzy:patch-1 branch from 508e8f9 to 8148bfc Aug 10, 2019

@ljharb

ljharb approved these changes Aug 10, 2019

Copy link
Collaborator

left a comment

This will only apply to non-variable-declaration cases, so I think it's reasonable to loosen it.

@ljharb ljharb merged commit 8148bfc into airbnb:master Aug 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@frenzzy frenzzy deleted the frenzzy:patch-1 branch Aug 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.