-
-
Notifications
You must be signed in to change notification settings - Fork 4
Improve generic types #3
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
Conversation
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
|
Thanks for the review 🙂 - I've applied your suggestions. |
wooorm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new behavior should be tested with tsd. Examples:
- https://github.com/syntax-tree/unist-util-visit/blob/main/index.test-d.ts
- https://github.com/syntax-tree/hast-util-is-element/blob/main/index.test-d.ts
It might be easier for generics to write a d.ts file manually, and use them in the JavaScript code. Examples:
| * @returns {Node} | ||
| * @template {Node} NodeLike | ||
| * @param {NodeLike} node | ||
| * @returns {NodeLike} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one’s broken? (it apparently already was?)
Hmm, fair point. I did find it pretty tricky to get the types right with just JSDoc, and I'm still not totally happy. I'll take another pass at this later. |
|
I spent some more time looking at this and came to the conclusion that it is extraordinarily difficult to type this function correctly. As such, I'm going to close this PR. Here's about as far as I got. I just can't find a way to make the output type of the mapping function dependent on the input type - and I'm not 100% sure that it's actually possible. |
This comment has been minimized.
This comment has been minimized.
|
Yeah, this stuff can get really complex. And also slow. Not for the faint of heart. ;) |
Initial checklist
Description of changes
This PR improves the JSDoc types that come with
unist-util-map.The existing type of
mapis not generic. This is fine if you just want to mapNodes toNodes, but painful if you want to do typesafe mapping of (e.g.)hast-specific node types in your TypeScript code. I came across this scenario while I was working on addressing some bugs in therehype-twemojifyplugin.This PR updates the type of
mapto the following effective type: