Docs: Add prefer-destructuring examples (fixes #9970)

[pvdlg]

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

[x] Documentation update
[ ] Bug fix (template)
[ ] 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:

What changes did you make? (Give an overview)

Add examples for the prefer-destructuring rules when enforceForRenamedProperties is enabled.

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

Nothing in particular.

[not-an-aardvark]

Thanks for the PR!

I think both of these samples are actually specific to the object option, not the array option. (ESLint doesn't know whether your variable actually contains an array -- it just sees a dynamic property access like foo[bar].)

[pvdlg]

I'm confused. In #9970, you mentioned that for accessing a specific index of an Array with a variable, when enforceForRenamedProperties is true, the workaround is to use const { [someIndex]: value } = array;. So I don't understand how it's not related to the array option...

[not-an-aardvark]

Syntactically, your property access from #9970 is the same as:

const array = { [someIndex]: 3, foo: 1, bar: 2 }; // an object with a `someIndex` key

const { [someIndex]: value } = array;

This is considered "object destructuring" because it's a syntax used for object property access, regardless of whether the value of the array variable is actually an array at runtime. (In this case, the square brackets are used to indicate a computed property key, and are unrelated to arrays.)

[pvdlg]

With the config "prefer-destructuring": ["error", {"array": true} the following is not considered an error:

var foo = array[someIndex];

And the following is considered an error:

var foo = array[0];

So it seems the rule for array has an exception case for when the index is a computed value or a variable.

But with the config "prefer-destructuring": ["error", {"array": true}, {"enforceForRenamedProperties": true} the following is considered an error:

var foo = array[someIndex];

So it seems that with enforceForRenamedProperties: true the exception case for indexes that are computed value or variable is not respected.
This is why I opened #9970. Is there something I'm misunderstanding?

Is your explanation in #9970 a way to workaround the array rule limitation by using object restructuring for an array?

[not-an-aardvark]

The array option applies when the computed value is an integer literal. It doesn't apply when the computed value is a variable or a different type of expression.

I think the confusion might be coming from the fact that both the array and object options default to true. So ["error", {"array": true}, {"enforceForRenamedProperties": true}] is equivalent to ["error", {"array": true, "object": true}, {"enforceForRenamedProperties": true}], which is why your last example is reporting an error. If you change that to "object": false, the error disappears. (Note that the error message for this case is "Use object destructuring" rather than "Use array destructuring", indicating that it is coming from the object option.)

[pvdlg]

Sorry for the late answer. After doing a bit more test, I found out that with the following config:

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

I get the error Use object destructuring for this code:

var array = [1,2,3];
var someIndex = 1;
var foo = array[someIndex];

I think this is the problematic (or at least confusing part). When the rule is configured for reporting errors related to object but not array and enforceForRenamedProperties is true then we get an error for accessing an array.

You mentioned:

The array option applies when the computed value is an integer literal. It doesn't apply when the computed value is a variable or a different type of expression.

But someIndex is an integer literal and it seems the object rule still applies. Isn't it a bug?

In addition shouldn't the array option applies when we destructure an Array independently of the type of index/prop?

For example:

var array = [1,2,3];
var someIndex = 1;
var foo = array[someIndex]; // => Apply the array rule as array is an Array

var obj = {a: 'value', b: 'value'};
var someProp = 'a';
var foo = obj[someProp]; => Apply the object rule as obj is an Object

If it's not possible to determine if an Object is an Array or not via the AST, and we have to rely on the type of someIndex/someProp then it still seems there is a bug as in the example above someIndex is an integer and someProp is a String even though in both case the Object rule is considered.

What's even more confusing is that this behavior happens only when enforceForRenamedProperties is true. If it's false it seems the the array option is considered only for Array (or for integer prop/index) and the object rule is applied only for Objects (or for String prop/index).

[not-an-aardvark]

But someIndex is an integer literal and it seems the object rule still applies. Isn't it a bug?

To be more precise, someIndex is a variable, not an integer literal. In your example, it's clear that the value of the someIndex variable will be an integer at runtime, and the value of the array variable will be an array at runtime, but JavaScript is a dynamically typed language, so in general it's not possible to determine the runtime type of any given variable without running the code.

For example, it wouldn't be possible to use array destructuring in this code:

function accessIndex(array, index) {
  const element = array[index];
}

• Could you use const [element] = array? No, because that would only work if index has a value of 0.
• Could you use const [, element] = array? No, because that would only work if index has a value of 1.

Hopefully, this example shows that array destructuring can only be used when the index is a known constant at development time.

You could use const { [index]: element } = array, but that would be considered object destructuring, not array destructuring.

I think the names "array destructuring" might be leading to some confusion. The term describes the syntax used to destructure a value (specifically the syntax that uses [), not the runtime type of the destructured value. Since JavaScript arrays are also considered to be objects, it is possible to use "object destructuring" on an array value.

[pvdlg]

Understood. It seems really difficult to handle this case as it's not possible to know what is the type of the variable someIndex/someProp not the type of array/obj.

The fact the rule offer the options array and object strongly suggest that it can be applied only to Array or only to non-Array Object. But it seems its not technically possible as there is no solution to figure out in the AST if and Object is an Array or not when an index is accessed via a variable.

The current implementation considers the array option only when accessing via an integer literal that is not a variable:

obj.prop; //Consider the object option => Make sense
obj[variable]; //Consider the object option => Make sense
array[1] //Consider the array option => Make sense
array[variable]; //Consider the object option => Confusing

Maybe the array and object option should be named something like integerLiteralAccess / nonIntegerLiteralAccess, or anything that convey the idea the first option is only considered for integer literal independently of the type being accessed (Array or non-Array Object)?

I'm not sure how to improve the documentation in the current situation. Adding example as I did in this PR is incorrect and don't make things less confusing.

It seems the following statements from the doc are inaccurate/misleading:

The first object parameter determines what types of destructuring the rule applies to.

The two properties, array and object, can be used to turn on or off the destructuring requirement for each of those types independently.

[platinumazure]

@not-an-aardvark @pvdlg Where are we at on this? Is this change still something we want to pursue?

[not-an-aardvark]

I think the current state is that the rule's behavior is causing some confusion which could be improved by adding something to the documentation, but this PR currently isn't what's needed.

I'm not sure it would be a good idea to rename the array option to integerLiteralAccess, because the rule's current suggestions with the array option generally only make sense if the value is actually an array. For example, in the expression var x = foo[0], if foo is not an array then var [x] = foo will probably throw an error.

[pvdlg]

Yes, I guess this PR can be closed. The way the rule works is confusing and I don't think that can be resolved by updating example as I initially thought.

[platinumazure]

Closing per this comment.

@pvdlg Thank you for the great discussion, and please do feel free to create an issue or submit another PR if you have more ideas on how to improve the documentation. We greatly appreciate all contributions!