Move @types/* packages to dev dependencies #322
Conversation
| @@ -75,6 +64,17 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "@sindresorhus/tsconfig": "0.7.0", | |||
| "@types/is-ci": "^2.0.0", | |||
| "@types/lodash.throttle": "^4.1.6", | |||
There was a problem hiding this comment.
You cannot move all types. This type, for example, is used in the source code:
Line 3 in 293d40f
There was a problem hiding this comment.
That is only used in the compiled js, not in the TypeScript definitions, so it's not required by this package at runtime nor at downstream compile time meaning it can be safely moved :)
There was a problem hiding this comment.
Ok I think I'm confused now. What's a question I need to ask myself to determine when a @types/* dependency should be included in dependencies or devDependencies? My understanding was that this question is "Are types from @types/* package re-exported from my module in any way?".
There was a problem hiding this comment.
"Are types from @types/* package re-exported from my module in any way?".
That is the correct question to ask.
I'm not sure why @sindresorhus thinks this type is required in dependencies, since it fails the above question.
There was a problem hiding this comment.
You need to keep at least react, node, and yoga-layout (the latter seems to ship its own types)
node should probably be defined as *
There was a problem hiding this comment.
Ok, In that case I think this PR is good to go as-is? None of the types from dependencies I've moved to devDependencies are exported from Ink.
There was a problem hiding this comment.
See #322 (comment), you need at least node and react - possibly optional peer deps tho
There was a problem hiding this comment.
@SimenB @types/node should be a dev dependency, and maybe also an optional peer dependency, although I usually don't do that as it's expected that the user has that anyway if they use TypeScript.
I think comment by @sindresorhus makes sense to me, if developer is using TypeScript, they most likely already have (and should) @types/node and @types/react installed.
There was a problem hiding this comment.
@milesj seems to share the same opinion on this too, sounds logical to me.
There was a problem hiding this comment.
Yeah, that's how I do it for most of my stuff. But if you want to add them, just do * versions to not conflict with consumer versions.
|
Thanks everyone for feedback, very useful! |
Fixes #320.
cc @G-Rath