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

feat: make types more specific #66

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

voxpelli
Copy link

@voxpelli voxpelli commented Mar 5, 2024

When looking a bit into eslint-community/eslint-utils#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 eslint-community/eslint-utils#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

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.

@nzakas
Copy link
Member

nzakas commented Mar 5, 2024

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 nzakas requested a review from a team March 5, 2024 17:43
@voxpelli
Copy link
Author

voxpelli commented Mar 5, 2024

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.

@mdjermanovic
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Feedback Needed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants