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.

Sign up for GitHub

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

Jump to bottom

Object destructuring on variable reassignment #6

Open
hiimjako opened this issue Mar 2, 2023 · 4 comments · May be fixed by #7
Open

Object destructuring on variable reassignment #6

hiimjako opened this issue Mar 2, 2023 · 4 comments · May be fixed by #7

Comments

@hiimjako
Copy link

hiimjako commented Mar 2, 2023

Hi, I have encountered unreadable behavior, in my opinion, when reassigning a variable from an object that contains a key with the same name as the variable, for example:

let foo = null
try {
  const response = await apiCall()
  foo = response.foo // eslint error: Use object destructuring.eslintprefer-destructuring
} catch (error) {
  console.error(error)
}

This behavior is managed by prefer-destructuring ESLint rule. The way it is currently set up, it forces you to adjust the code like this:

let foo = null
try {
  const response = await apiCall();
  ({ foo } = response.foo) // No eslint errors
} catch (error) {
  console.error(error)
}

I was wondering if it wouldn't be more readable to adjust the rule to allow the reassignment as shown in the first example.
It could be modified by updating the rule like this:

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

This would not be a breaking change, because both syntaxes will work without throwing ESLint errors.
@davidebianchi
Copy link
Member

Hi @hiimjako, I think this change it's ok. For sure, it is better the assignment.
Can you add a PR?

@cescobaz
Copy link

cescobaz commented Mar 3, 2023

I think that linter is suggesting the following:

const { foo } = await apiCall();

You don't need the response variable, in my opinion it is clean.

@davidebianchi
Copy link
Member

Yes, sure this could be done. In this case where you want to set in a variable, you should have another variable name instead.
Here we could talk about if the try/catch clause with external reference is a good pattern to use, but it's sure better than nested try/catch clause imho.

@hiimjako
Copy link
Author

hiimjako commented Mar 3, 2023

I think that linter is suggesting the following:

const { foo } = await apiCall();

You don't need the response variable, in my opinion it is clean.

It is not either possible, for this reasons:

  1. In the example provided in the issue I'm not declaring the variable, it is already declared and I am reassigning it;
  2. You can do something like this
({ foo } = await apiCall())

But if you have a previous statement, you have to use a semicolumn;
3. As shown in step 2, you have to put it inside the parentheses and I think it is better to avoid this syntax if possible in this cases.

As said by @davidebianchi you should have another variable name to do that, but to me, it appears a workaround changing variables names to satisfy linter.

@hiimjako hiimjako linked a pull request Mar 3, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow variable reassignment without object destructuring mia-platform/eslint-config-mia
3 participants
@cescobaz @davidebianchi @hiimjako