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

consistent-destructuring improvements #1005

Open
zloirock opened this issue Jan 11, 2021 · 9 comments
Open

consistent-destructuring improvements #1005

zloirock opened this issue Jan 11, 2021 · 9 comments

Comments

@zloirock
Copy link
Contributor

It could be good to add an option for ignoring redefined properties of sources:

// I don't see a reason to use destructuring for `edge`
const { chrome, ie, safari } = module;
// ...
if (!module.edge && ie) {
  module.edge = '12';
}
// ...
if (!module.edge) {
  // ...
}

It could be good to add an option to ignore some sources or properties:

const { getPrototypeOf } = Object;
// ...
// `Object.prototype` here looks better than just `prototype`
if (getPrototypeOf(object) === Object.prototype) {
  // ...
}
@brettz9
Copy link
Contributor

brettz9 commented Jan 12, 2021

Another improvement, imo, would be to allow referencing a higher property that was deeply destructured:

const {request: {cache, mode, url}} = e;
doSomething(cache, mode, url);

doAnother(e.request);

@GuoSirius
Copy link

Another improvement, aim at Vue@3.0,It's no longer responsive when you deconstruct it

<script>
import { defineComponent, watch } from 'vue'

export default defineComponent({
  name: 'Hello',
  setup(props, context) {
    const { a, b, c } = props

    if (a && b && c) {
      // TODO
    }

    watch(() => props.a, () => {})
    watch(() => props.d, () => {})
  }
})
</script>

@GuoSirius
Copy link

asyncRequest(result => {
  const { a, b, c, d = [] } = result

  const e = result.e || []

  // The value of d and e is different
})

@sindresorhus
Copy link
Owner

// @medusalix @nickofthyme @MrHen @futpib Mentioning you from the original issue for the rule in case you have any thoughts on these possible improvements.

@fisker
Copy link
Collaborator

fisker commented Apr 28, 2021

// @zloirock @brettz9 @GuoSirius Can you take a look my new proposal #1230, I think this is more like you want.

@brettz9
Copy link
Contributor

brettz9 commented Apr 28, 2021

Regarding your Vue case, @GuoSirius , I think that could be met by https://eslint.vuejs.org/rules/no-setup-props-destructure.html .

@futpib
Copy link
Contributor

futpib commented Apr 28, 2021

I think examples provided here can be rewritten in a better way that does conform to the latest (31.0.0) unicorn rules:

@zloirock's from #1005 (comment):

let {chrome, ie, safari, edge} = module;

if (!edge && ie) {
	edge = '12';
}

if (!edge) {
	// ...
}

// If you *really* need to mutate `module.edge` for some reason,
// do it after you changed all destructured variables you wanted to change:
Object.assign(module, {
	chrome,
	ie,
	safari,
	edge,
});
if (Object.getPrototypeOf(object) === Object.prototype) {
	// ...
}

@brettz9's from #1005 (comment)

This already passes unicorn rules, but you could rewrite it to use even more destructuring with IMO even better readability:

const {request} = event;
const {cache, mode, url} = request;

doSomething(cache, mode, url);
doAnother(request);

I don't get exactly which improvements are proposed here. I think this needs more clear examples with comments like "This line is reported (and/or autofixed) but I think it should not be reported" or "This line is not reported but I think it should be reported (and/or autofixed)" each in a separate issue. Even better, if you feel like it, write a failing test for maximum clarity.

@brettz9
Copy link
Contributor

brettz9 commented Apr 28, 2021

Yes, they can be redone, but some of us consider it troublesome to have to refactor, e.g., changing const to let, just to get it to work. Also, in my case, I'm aware I could make those steps, but I don't tend to wish to add lines for just a one-off use (and unicorn/consistent-destructuring does report this).

@brettz9
Copy link
Contributor

brettz9 commented Apr 28, 2021

I might just add that I think there are essentially three approaches that are favored, none of which is necessarily superior, but the current rule only allows one of them.

  1. Require it everywhere possible (the existing rule)
  2. Require it wherever a destructuring structure has already exposed the specific variable (@fisker's proposal)
  3. Require it wherever a destructuring structure currently makes it feasible (my own preference)

By my own preference, I mean the following:

const {a, b, c: {d}} = obj

obj.c // Should *not* report since it would require retooling (a new line)
if (!obj.f) { // Should *not* report as it would require retooling (replacing
                 //    with `let` or a `let` and `const`)
  obj.f = '12';
}
obj.e // Should report since it can just be added to the destructuring

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

No branches or pull requests

7 participants