feat: add JSXSpreadChild and tool to build keys out of AST definitions

[brettz9]

Refiling with modifications from original PR #35.

This PR adds:

1. chore: tool to build keys out of AST definitions
2. In the process of making these changes (and using the approach advised by @mdjermanovic to look at interface type rather than name), the following are currently still changed:
   1. Breaking Change: Removes ExperimentalRestProperty, ExperimentalSpreadProperty
   2. feat: Adds JSXSpreadChild
3. refactor: Alphabetizes the Node types for easier comparisons (though the ordering of their keys remain unchanged). (Could be breaking if Node type iteration order were significant though I wouldn't expect this to be the same issue as with keys since the Node types seem to be more of a map.)

Here are the items on which I think I in particular still need feedback or action:

• Whether to force-add the Experimental* properties or just let them be dropped as per the above (and confirm that JSXSpreadChild should indeed be added).

• I currently allow two approaches in the builder toward restricting output: suppressing by a given key name or by making an interface non-traverseable. For example, loc, comments, innerComments, and operator are currently added to the properties suppressed by getKeys (parent, leadingComments, and trailingComments) in order to ensure they continue not to appear in the listing. (This also raises another question. See next item.) I could alternatively suppress the SourceLocation, Comment, and *Operator types from being treated as traverseable (and thus properties of these types would not be added as keys). But even if it doesn't matter the approach, I still need to know if my suppressing those properties was suitable.

Besides the impact on keys, the Nodes that are added are affected. I'm currently not including SourceLocation (it is not listed as an exportable interface, doesn't have a type, and was not included previously), nor am I including Line and Block type comment Nodes despite their having exportable AST available, as this had not previously been included (and they don't seem to be accepted by ESTree), though I can do so if desired. See also discussion below on comments.

• Whether getKeys needs to suppress the above-mentioned keys also, and also suppress type since the Node argument supplied by the user presumably may have a type on it.

While it would seem to me from my limited perspective that it would be better to err on the side of including the comment info, as I imagine people might have a desire to traverse or query this separately, if that were to be the case, it raises the ESTree question for me of why non-block nodes could not have comment properties; in TypeScript, for example, such inline comments are often used for casting. Similarly, I would think it would be ideal to have a standard for JSDoc-block attachment even if as a mere optional extension to the standard, e.g., an optional jsdoc property associated with JSDocBlock, JSDocTag, the various types, etc. (as we do with https://github.com/es-joy/jsdoccomment/blob/main/src/commentParserToESTree.js#L237-L242 and https://github.com/jsdoc-type-pratt-parser/jsdoc-type-pratt-parser/blob/34bd2a6e72af6b16b47e4e1a4391142035917e89/src/visitorKeys.ts#L7-L35 or as it appears TypeScript may be doing: https://github.com/typescript-eslint/typescript-eslint/blob/550b61ee1096113b234bf035dd267ba385809961/packages/typescript-estree/src/ts-estree/ts-nodes.ts#L184-L208 ). I could file this at ESTree, but mentioning here as the PR raises the question.

• I'll wait on adding full build tool coverage until knowing the final expectations.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Brett Zamir (c57689f, 8739546, ee6fc73)

[brettz9]

I'm not clear on why the CLA is failing as GitHub shows my user next to the commit.

[nzakas]

Double check that the email associated with the commit is one associated with your GitHub user account.

[brettz9]

Ah, had had an ESLint error and ESLint Jenkins had been the actual author. Thanks!

[brettz9]

Ok, so to break down the next question of my OP perhaps a little more digestibly:

Based on the previous pattern of our keys, I understand that we do not want loc, comments, innerComments, and operator to show up, even though they do have, like other properties we do want, their own interface (i.e., Comment and SourceLocation) or type (the *Operator string union types like UnaryOperator) and as such would normally be expected to be traversable by the algorithm I have set up.

I didn't settle definitively on one or the other. I can go solely with either of a few approaches of determining how these properties should be ignored and would like to know which one or ones you all think is best for me to use:

1. Explicitly suppressing the undesired loc, comments, innerComments, and operator properties above and beyond those suppressed by getKeys.
2. Adding loc, comments, innerComments, and operator to the current explicit exclusions made by getKeys (i.e., excluding these new ones along with the existing parent, leadingComments, and trailingComments that comprise the KEY_BLACKLIST).
3. Suppress the SourceLocation and Comment interfaces, and *Operator types from being treated as traverseable (and thus properties of these would not be added as keys)

Currently, the results would be the same. but it seems, all things being equal, that it may be more conceptually appealing to use one consistent approach.

No. 1 and 2 seems they would be riskier if these names might legitimately need traversal in some new interface. Although no. 3 seems the most satisfying to me personally as it confines the exclusions not by property name but by interfaces considered not to be traversed, no. 2 seems most consistent with our current approach, given that getKeys is excluding specific properties regardless of the node type that contains them.

But even if choosing no. 3, I wonder whether no. 2 should be implemented so that our getKeys also excludes the undesired properties when encountered. I could do so by autogenerating our KEY_BLACKLIST as well as our KEYS (though parent would need to be manually added since the AST doesn't specify parent so as to allow it to be programmatically detected somehow; the other two currently in the blacklist, leadingComments and trailingComments, could be excluded based on their pointing to an array of the undesired Comment).

[nzakas]

LGTM. Thanks! Just want another set of eyes before merging.

[nzakas]

Oops, forgot this was a WIP. Whenever you’re ready, feel free to submit for approval.

[brettz9]

@nzakas The PR is indeed close to being ready (mostly just need some tests), but really feel I need a decision on #36 (comment) first.

To summarize that admittedly long comment, we need to decide between the approaches to:

1. Blacklist undesired properties through our build routine only
2. Blacklist undesired properties by adding to the key blacklist of getKeys
3. Suppress specific interfaces/types which we know hold the properties we don't want

I supported a mix of algorithms in the event we needed them, but I think as per that comment, we probably only want one (I guess the second one).

Once I know which algorithm to use, I can adapt the code and add tests.

[nzakas]

I think the second one makes sense 👍

[brettz9]

Ok, it should now be ready.

[mdjermanovic]

How should we tag this change? It shouldn't be just a chore because it includes some changes in the functionalities of this package:

• It adds JSXSpreadChild (that alone would be feat ?)
• It changes the public getKeys() (breaking feat! ?)

[brettz9]

Might the getKeys breaking change, be more of a fix? I would think that this should have been already excluding those properties (loc, comments, innerComments, and operator).

[mdjermanovic]

Might the getKeys breaking change, be more of a fix? I would think that this should have been already excluding those properties (loc, comments, innerComments, and operator).

This is a separate package, so it might be correct to do a major version bump when a change seems potentially breaking for packages that depend on it even if the change isn't breaking for ESLint.

As for ESLint, I think everything related to comments can be classified as a fix. They're objects with a "type" property, so they would be mistakenly traversed as nodes if properties that point to them were not excluded.

loc makes sense as a performance improvement since it's a known ESTree property that exists on all nodes and doesn't point to other nodes. Maybe it would make sense, for the same reason, to also add type (I'm not sure why it wasn't added before, and even the README example shows that getKeys() returns "type"), and range although it isn't in the ESTree spec.

operator seems like an outlier - other excluded properties are either known properties that have a specific meaning (parent, loc) or properties related to comments (comments, leadingComments, innerComments, trailingComments) . I'm not sure if a generic fallback function should assume that operator child is not a node.

[brettz9]

They're objects with a "type" property, so they would be mistakenly traversed as nodes if properties that point to them were not excluded.

Ah, there's the missing ingredient, thanks! I had been so focused on the AST, I neglected to dig into the traversal routine to discover the exact specification of the items to be iterated.

Might the getKeys breaking change, be more of a fix? I would think that this should have been already excluding those properties (loc, comments, innerComments, and operator).

This is a separate package, so it might be correct to do a major version bump when a change seems potentially breaking for packages that depend on it even if the change isn't breaking for ESLint.

Ok, in that case, and based on the information above, whether or not we add the excluded keys later to getKeys in some breaking release for such purposes as optimization and clarity, I think I will need to tweak the algorithm to exclude properties which do not point to objects with a type or those whose type is "Line" or "Comment". That will also take care of operator which resolves to a union of strings instead of an object.

My existing routine should help, but will take some time to add this detection.

loc makes sense as a performance improvement since it's a known ESTree property that exists on all nodes and doesn't point to other nodes.

Ok. I guess we need a new issue then, especially since I don't actually need to add to getKeys in this PR.

Maybe it would make sense, for the same reason, to also add type (I'm not sure why it wasn't added before, and even the README example shows that getKeys() returns "type"), and range although it isn't in the ESTree spec.

Yeah, I was wondering that re: type too. And yes, range.

operator seems like an outlier - other excluded properties are either known properties that have a specific meaning (parent, loc) or properties related to comments (comments, leadingComments, innerComments, trailingComments) . I'm not sure if a generic fallback function should assume that operator child is not a node.

Ok, sure. As per the above, I should be able to get the algorithm to detect the non-traversability of operator (usually properties with independent types were traversable but not in this case).

[brettz9]

Ok, the detection should now be ready without any breaking changes. But JSXSpreadChild is added, so indeed apparently a feat as discussed.

[nzakas]

@mdjermanovic can you take a look?

[mdjermanovic]

Can we remove these lines? We usually avoid having commented-out code in the codebase unless it is associated with a TODO action.

[brettz9]

Sure. I had thought these were going to be removed in a future version, so should I add a comment to do so? File an issue? Is there any particular approach for planning ahead with to-dos to occur before breaking changes?

eslint-plugin-unicorn has a truly excellent expiring-todo-comments rule which can be tied to version or dependency changes which will cause linting to fail unless applied. I might recommend unicorn for eslint-config-eslint anyways.

[mdjermanovic]

I think it would be best to discuss this in an issue first.

[mdjermanovic]

Sure. I had thought these were going to be removed in a future version, so should I add a comment to do so? File an issue? Is there any particular approach for planning ahead with to-dos to occur before breaking changes?

We are using GitHub issues to track planned features and bug fixes. We are rarely using TODO comments, mostly for code parts that we think should be refactored.

[brettz9]

Ok, thanks for the feedback. Added #38 and removed the comments.

And FWIW, re: Unicorn, I filed eslint/eslint#15731 . Even when not heavily using to-dos in code, having those conditional to-dos can really help ensure that version-dependent to-dos don't get lost.

[mdjermanovic]

LGTM, thanks!

[nzakas]

LGTM. Thanks for all the work on this.