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 type dependencies #42

Merged
merged 2 commits into from
May 19, 2020
Merged

Add type dependencies #42

merged 2 commits into from
May 19, 2020

Conversation

remcohaszing
Copy link
Member

The dependencies on @types/unist and @types/mdast are needed to consume
this package in TypeScript.

If this is not defined, all dependants need to define this dependency.

This would fix the build for remarkjs/remark-rehype#13 and all sava a lot of hassle for its dependants.

The dependencies on `@types/unist` and `@types/mdast` are needed to consume
this package in TypeScript.

If this is not defined, all dependants need to define this dependency.
@wooorm
Copy link
Member

wooorm commented May 16, 2020

😬 I thought it was the plan to not do typescript dependencies, but keep them in dev deps, so non-ts users (which are many) don’t have to download that? I’ve lost the context.

@remcohaszing
Copy link
Member Author

The context has been spread across multiple projects and pull requests, and spectrum. Putting types in devDependencies has proven to be troublesome, as can be seen in remarkjs/remark-rehype#13.

I think it would be best to discuss this in a centralized location, not this issue, which is one of many TypeScript related issues.

I found an old post on Spectrum that’s related. Although feedback on my several PRs seems to mention the exact opposite of the conclusion there. Should this be discussed further in that post?

@wooorm
Copy link
Member

wooorm commented May 16, 2020

What’s troublesome about that linked issue? The missing types can be installed as a dev dep, no?

I agree. Either Spectrum, or the linked issue: syntax-tree/unist-util-is#14. I was under the impressions that deps was the wrong way to go, dev-deps okay, and potentially also peer-deps.

@ChristianMurphy
Copy link
Member

The issue that prompted putting all @types in devDependencies was unifiedjs/unified#45.
With vfile/vfile#54 in place, that issue should be resolved 🤞 😬 .
I'd be open to trying dependencies again.

@ChristianMurphy ChristianMurphy added the ☂️ area/types This affects typings label May 16, 2020
@wooorm
Copy link
Member

wooorm commented May 18, 2020

Alright, I’d rather have no types for non-ts users, but let’s try it out!

@wooorm
Copy link
Member

wooorm commented May 18, 2020

@remcohaszing @ChristianMurphy patch, minor, major?

@ChristianMurphy
Copy link
Member

I think minor, this adds dependencies, but shouldn't break the API. 🤞

@wooorm wooorm added 📦 area/deps This affects dependencies 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change labels May 19, 2020
@wooorm wooorm merged commit 960a349 into syntax-tree:master May 19, 2020
@wooorm
Copy link
Member

wooorm commented May 19, 2020

Released as 9.1.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 area/deps This affects dependencies ☂️ area/types This affects typings 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

3 participants