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

Move @types/tmp to production dependency #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move @types/tmp to production dependency #38

wants to merge 1 commit into from

Conversation

yoursunny
Copy link

Several types from @types/tmp package, such as FileOptions and DirOptions, are exposed as part of public typing in index.d.ts. This requires @types/tmp to be a production dependency instead of a devDependency, so that TypeScript consumers can find the correct typing for these options instead of any.

@paulmelnikow
Copy link
Collaborator

Hi! Thanks for this. This was discussed at length in #31, care to add to that?

@yoursunny
Copy link
Author

The problem now is that tmp-promise does not have correct typings, so it's completely unusable in TypeScript.
An alternative from adding a production dependency is copying the relevant types into index.d.ts. Do you want to try that approach?

@benjamingr
Copy link
Owner

@yoursunny you do type checking in production rather than as part of your build? Is typescript itself a dependency rather than a devDependency for you?

@yoursunny
Copy link
Author

The problem here is that, when I run npm install --save tmp-promise, I should be able to do type checking. However, without having @types/tmp as a production dependency, NPM would not install @types/tmp package, and thus type checking is not working for the argument to dir() function.

yoursunny added a commit to yoursunny/NDNts that referenced this pull request Oct 13, 2019
@bitomic
Copy link

bitomic commented Jan 14, 2022

It's been already two years for this issue, haven't you changed your mind? 😅

I am pretty sure that you are aware that, e.g. for the file exported function you have the following typings:

function file(options?: any): Promise<tmp.FileResult>

That any type for options is... quite not-so-useful, don't you think? It is providing partial support for TypeScript to the point it may be better for all parties to have @types/tmp-promise as a separate package, in my opinion.

@benjamingr
Copy link
Owner

benjamingr commented Jan 14, 2022

PRs to improve types are welcome (or just port it to TypeScript and export the d.t.s - should be pretty easy). I am happy to guide such work and how I would do it.

Also just updating the types in the index.d.ts file to add what's missing, PRs are likely to get merged quickly with improvements for that.

That is unrelated to @types/tmp being a production dependency since types don't belong in production dependencies :)

@benjamingr
Copy link
Owner

It is providing partial support for TypeScript to the point it may be better for all parties to have @types/tmp-promise as a separate package, in my opinion.

This also sounds good to me

@pauldraper
Copy link

pauldraper commented Apr 9, 2023

types don't belong in production dependencies :)

If you are shipping the types with the production library (which tmp-promise is doing), then the dependency types belong in dependencies of that package.

If you are shipping the types separately from the production library (which tmp does), then the dependency types belong in dependencies of that types package.


Like, yes it is a couple extra bytes. But you're already having a couple extra bytes by shipping code+types together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants