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

Add explicit dependency on @types/unist #146

Merged
merged 2 commits into from
Jul 6, 2021

Conversation

GuiltyDolphin
Copy link
Contributor

@GuiltyDolphin GuiltyDolphin commented Jul 5, 2021

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

This PR adds the latest version of @types/unist an explicit dependency (as we import from it directly).

Edit: I notice in the description that dependency on @types/unist is suggested for projects using typescript - is there a particular downside to depending on it directly here?

We import directly from 'unist', so it should be make an explicit
dependency. This ensures we are using the latest version of unist.
@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 Jul 5, 2021
@GuiltyDolphin
Copy link
Contributor Author

GuiltyDolphin commented Jul 5, 2021

cc @wooorm @remcohaszing @ChristianMurphy

Should I bump unified's (patch?) version number too, so the change in typing doesn't affect packages depending on the old system?

@GuiltyDolphin
Copy link
Contributor Author

Btw @ChristianMurphy @wooorm I'm working through the main unified packages to update @types/unist to the latest version. Because it is (technically) a breaking change, updating forces us to make sure we aren't expecting types in the old format, and for going forwards.

I've done a few repos (for the most part, the code doesn't need changing, just the dependencies), but I want to check with you both that I'm doing the correct thing before I go ahead with the others.

package.json Outdated Show resolved Hide resolved
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
@codecov-commenter

This comment has been minimized.

@ChristianMurphy
Copy link
Member

Btw @ChristianMurphy @wooorm I'm working through the main unified packages to update @types/unist to the latest version. Because it is (technically) a breaking change, updating forces us to make sure we aren't expecting types in the old format, and for going forwards.

I've done a few repos (for the most part, the code doesn't need changing, just the dependencies), but I want to check with you both that I'm doing the correct thing before I go ahead with the others.

Thanks @GuiltyDolphin!
In general it is worth checking, looking for breaking changes.
We try to keep version ranges wide, for example ^2.0.0 rather than ^2.1.3 for example.
Which means not many repos will need their package.json updated, but a few may need their test suite or typings adjusted.

@GuiltyDolphin
Copy link
Contributor Author

Alright! I'll keep checking through then, but without updating version numbers and just make sure that typings are okay for the latest version of @types/unist. It looks like we're mostly okay though, not too many packages are making assumptions about extra keys on Node. 😄

@wooorm wooorm changed the title Add explicit dependency on latest version of unist Add explicit dependency on @types/unist Jul 6, 2021
@wooorm wooorm merged commit 2c7ba99 into unifiedjs:main Jul 6, 2021
@wooorm wooorm added the 💪 phase/solved Post is done label Jul 6, 2021
@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

5 participants