Skip to content

Fix TypeScript node16 and ESM#12

Merged
wooorm merged 3 commits intosyntax-tree:mainfrom
alecmev:esm
Aug 21, 2022
Merged

Fix TypeScript node16 and ESM#12
wooorm merged 3 commits intosyntax-tree:mainfrom
alecmev:esm

Conversation

@alecmev
Copy link
Contributor

@alecmev alecmev commented Aug 21, 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

See syntax-tree/mdast-util-mdxjs-esm#3.

Blocks syntax-tree/unist-util-visit#35.

@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 Aug 21, 2022
Copy link
Member

@JounQin JounQin left a comment

Choose a reason for hiding this comment

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

Other parts LGTM.

package.json Outdated
"tsd": "^0.22.0",
"type-coverage": "^2.0.0",
"typescript": "^4.0.0",
"typescript": ">=4.7",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"typescript": ">=4.7",
"typescript": "^4.0.0",

Can you revert this?
The style we use here is to have relaxed ranges except when they are really needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's truly needed in this case. It would allow TS 4.6 and older otherwise, which can't build this project.

Copy link
Member

Choose a reason for hiding this comment

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

It’s not needed. The TypeScript dependency isn’t different from any other. Version ^4.0.0 will resolve to TypeScript 4.7 (at the moment of writing). If you run into issues, try deleting node_modules and running npm install.

@wooorm wooorm requested a review from JounQin August 21, 2022 10:04
@alecmev
Copy link
Contributor Author

alecmev commented Aug 21, 2022

Don't merge yet, please!

@wooorm
Copy link
Member

wooorm commented Aug 21, 2022

You mean the range? I find both fine. ^4.7.0 or ^4.0.0.

@alecmev
Copy link
Contributor Author

alecmev commented Aug 21, 2022

Ah, I refer to the re-exporting of types, it isn't working as intended, figuring out a better solution.

@wooorm
Copy link
Member

wooorm commented Aug 21, 2022

ahh, right, thanks for spotting that, I was mixing the conversations on two PRs!

@alecmev
Copy link
Contributor Author

alecmev commented Aug 21, 2022

No worries! Tests pass, ready to be merged now.

@wooorm wooorm changed the title Fix ESM Fix TypeScript node16 and ESM Aug 21, 2022
@wooorm wooorm merged commit 348f2f6 into syntax-tree:main Aug 21, 2022
@wooorm wooorm added 🐛 type/bug This is a problem ☂️ area/types This affects typings 💪 phase/solved Post is done labels Aug 21, 2022
@alecmev alecmev deleted the esm branch August 21, 2022 10:34
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Aug 21, 2022
@alecmev
Copy link
Contributor Author

alecmev commented Aug 21, 2022

Thank you!

@wooorm
Copy link
Member

wooorm commented Aug 21, 2022

Thanks, released!

wooorm pushed a commit to syntax-tree/unist-util-visit that referenced this pull request Aug 21, 2022
Closes GH-35.
Related-to: syntax-tree/mdast-util-mdxjs-esm#3.
Related-to: syntax-tree/unist-util-visit-parents#12.

Reviewed-by: JounQin <admin@1stg.me>
Reviewed-by: Titus Wormer <tituswormer@gmail.com>
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 🐛 type/bug This is a problem

Development

Successfully merging this pull request may close these issues.

4 participants