eslint-visitor-keys icon indicating copy to clipboard operation
eslint-visitor-keys copied to clipboard

feat: make types more specific

Open voxpelli opened this issue 1 year ago • 3 comments
trafficstars

When looking a bit into https://github.com/eslint-community/eslint-utils/issues/150 I felt that the types of this module, which is used by that module, was quite generic – so I stepped into that rabbit hole and looked into if it could be made more tightly typed, and this is the result.

Is it strictly needed for https://github.com/eslint-community/eslint-utils/issues/150 ? Not really.

Would it be a waste to throw it away just because of that? Yes, I think so.

Exports

  • KEYS is now typed with exactly the structure that it has, thanks to using @satisfy and @type {const}. So if I access KEYS.ArrayPattern it has the type of readonly ["elements"]
  • getKeys() is now typed so that it returns a filtered union of string literals, based on the keys of the object sent in, rather than always just readonly string[]. It still returns readonly string[] when given an input that's eg Record<string, any>
  • unionWith() now uses the extended VisitorKeys to return VisitorKeys with the keys set to union of the keys in additionalKeys and VisitorKeyTypes (the keys of KEYS). It still returns plain VisitorKeys when given a generic input like VisitorKeys

Types

  • VisitorKeys is now a generic that can be used to create a more narrow type, like VisitorKeys<VisitorKeyTypes, import("estree").Node | import("estree-jsx").Node> would roughly be the same as what KEYS has (but less precise since keys gets all property types). Without any parameters given it works like VisitorKeys did previously.
  • Two new types:
    • VisitorKeyTypes, that represents all keys from KEYS
    • FilteredKeysOf<T>, the type equivalent to the filtering that getKeys() does

Additional notes

  • I replaced the readonly keyword in all places where it causes linting troubles (https://github.com/jsdoc-type-pratt-parser/jsdoc-type-pratt-parser/issues/164). It got replaced with either Readonly<> or ReadonlyArray<>.
  • Two new challenges with the jsdoc-type-pratt-parser arose though, so there are still some eslint-disable jsdoc/valid-types
  • I had to update the typescript/tsd and rollup modules to get it to eg. recognize the lib: ['es2022'] in the tsconfig.json

Closing notes

I realize that some of the changes proposed here might be seen as a bit extreme when it comes to juggling TypeScript types in JSDoc definitions – and if some are considered too extreme I can try to pull them out. But I think it's nothing too crazy in there.

voxpelli avatar Mar 05 '24 16:03 voxpelli

Interesting. This seems like overkill for such a simple package. I'm not sure how valuable these changes are based on the additional code that, honestly, makes it harder for me to read. Leaving open to see what the rest of the team thinks.

nzakas avatar Mar 05 '24 17:03 nzakas

I'm happy to go through and provide alternatives, justification and/or try to drop parts wherever you are interested.

I do agree this PR may have gone on the overkill-side part of things, so we can for sure scale back.

voxpelli avatar Mar 05 '24 18:03 voxpelli

I'm also not sure how useful is this. KEYS and other exports are meant to be used in generic traversals, so generic types like string[] seem right for this purpose.

mdjermanovic avatar Mar 05 '24 18:03 mdjermanovic

Closing as we decided not to move forward with this.

nzakas avatar Aug 20 '24 14:08 nzakas