Skip to content

Fix to add MapFunction type back #7

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

Merged
merged 3 commits into from
Sep 28, 2022
Merged

Fix to add MapFunction type back #7

merged 3 commits into from
Sep 28, 2022

Conversation

JounQin
Copy link
Member

@JounQin JounQin commented Sep 25, 2022

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

close #6

@JounQin JounQin requested a review from wooorm September 25, 2022 10:14
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Sep 25, 2022
@JounQin JounQin added ☂️ area/types This affects typings and removed 🤞 phase/open Post is being triaged manually labels Sep 25, 2022
@github-actions

This comment has been minimized.

@JounQin JounQin added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually labels Sep 25, 2022
@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 🤞 phase/open Post is being triaged manually 👋 phase/new Post is being triaged automatically labels Sep 25, 2022
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JounQin!
Code looks fine, it may be worth waiting for a resolution on #6 (comment) before moving forward with this.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool that this works from an index.js. I didn’t know that.

Can you remove from unist-util-map/complex-types.d.ts in the readme as well?

For later, optional improvements:

@JounQin
Copy link
Member Author

JounQin commented Sep 27, 2022

For later, optional improvements

  1. I'm not familiar with that part, so another PR can be raised for it.
  2. The asIs mapper with import('./index.js').MapFunction<Root> alreay tests part of it, but sure a index.test-d.ts will be much better.

@JounQin JounQin changed the title fix: add types Parent/MapFunction back fix: add type MapFunction back Sep 27, 2022
@JounQin JounQin requested a review from wooorm September 27, 2022 09:47
Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your hard work! Looks good to me, if other people think it’s fine to add TS 4.5+ syntax: #6 (comment)

Copy link
Member

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to try and stick to the DefinitelyTyped support window (unless it's really inconvenient). However, I believe this only requires TypeScript 4.5 to build the types from JSDoc right? Since the types can still be consumed using the same TypeScript versions, it's non-breaking.

@wooorm
Copy link
Member

wooorm commented Sep 27, 2022

@remcohaszing i was thinking that first, but without skipLibCheck, I was thinking maybe they'd get checked anyway?
Maybe you're right, and if that's the case then it's especially fine!

@remcohaszing
Copy link
Member

Nope, TypeScript only looks at the .d.ts files. It's the whole reason we build them 😅

@wooorm wooorm changed the title fix: add type MapFunction back Fix to add MapFunction type back Sep 28, 2022
@wooorm wooorm merged commit 5b1c636 into syntax-tree:main Sep 28, 2022
@wooorm wooorm added the 💪 phase/solved Post is done label Sep 28, 2022
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Sep 28, 2022
@wooorm
Copy link
Member

wooorm commented Sep 28, 2022

Released in 3.1.2! Thanks @JounQin!

@JounQin JounQin deleted the fix/types branch September 28, 2022 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

unist-util-map types Node and MapFunction stopped to be exported between v3 and v3.1.1
4 participants