-
Notifications
You must be signed in to change notification settings - Fork 93
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
Switching to Typescript #157
Comments
my humble proposal would be to provide typescript typings, e.g . |
That is an option of course, but switching to TypeScript would solve other problems too. When I was doing the switch I've found a couple of issues which were reported later. So it has other benefits too. |
@raszi I am open to that change. I am currently learning typescript and also have a setup here for gulp (also using typescript here) and mocha test cases implemented in typescript and so on... |
@raszi Please have a look at the branch https://github.com/raszi/node-tmp/tree/gulp-ts. Here you can find the dependencies and scripts that I use for my typescript based projects including gulp for the build process, working coverage reports and mocha based tests. This still needs more work and must be further customised for node-tmp. |
My2cents: From my own experience, making manual Also, a |
@jmendiara I hear you. However, I think I have reduced the required dependencies to a minimum. In addition, the gulp tasks themselves are simple and maintainable and once realised, would not need any further maintenance work unless, of course, gulp finally manages to get 4.x out.
Of course, the above dependencies have a lot of other dev dependencies, but I myself can live with that. (maven/npm downloading the internet again, have a beer 🥂 ). |
@raszi from what I have learned, we have to provide a complete set of typings for the tmp api, regardless of whether the code base is pure javascript or transpiled typescript. Either way, the provisioning of the typings and leaving the source as is would seem to be the best choice for an initial migration towards typescript. |
Now that I have gained confidence in TypeScript and I see that others already have provided typings for node-tmp under the hood of the DefinitelyTyped project on github, I am also open to switch to TypeScript as a whole. However, we still need to provide the typings since we will transpile to standard JS and publish the package as such. And while we do not have to switch to TypeScript at all, we might want to provide these type declarations as part of the node-tmp package. Yet, and since the package will not change any more unless we come up with some clever scheme of how doing things differently, I also see no urgent need in doing so. @raszi Your call. |
I believe it would be better to switch to TypeScript all together. The last time I've checked the provided type definitions those were not easy to use and were not entirely correct. |
Ok, well then it is TypeScript. Will you make the necessary changes? |
I think that we need to move lib/tmp.js to src/tmp.ts and distribute it via dist/tmp.js. The typings should then reside in a dist/tmp.d.ts, copied over from typings/tmp.d.ts or something like that. What do you think or do you already have a plan for making this work? |
Hi! Has there been any movement on this? Nock recently started shipping its types along with the library, and the benefit is that new features (including features in beta branches) can immediately show the correct types, and there's no need to worry about matching the version of the types to the version of the library. I wonder if this library might do the same, as an interim step before the TypeScript port is completed. I work on tmp-promise which is a mid-level dependency which ships types which depend on @types/tmp. We're in a bit of a jam (see benjamingr/tmp-promise#38 and benjamingr/tmp-promise#31) where either we need to declare @types/tmp as a dependency of tmp-promise, or else require developers to identify the correct version and declare their own dependency on @types/tmp. If tmp shipped its own types, that would be the simplest and most reliable solution. It also is a better way to ensure the types are correct and match the intention of the source; the review process in DefinitelyTyped is sort of stopgap, by comparison. I'd be happy to open a PR if there's interest in pulling the types in, as an interim solution to a TypeScript port. (Tangentially related: at some point it'd be nice to merge tmp-promise into this library, as discussed at benjamingr/tmp-promise#36!) |
@paulmelnikow there is https://www.npmjs.com/package/@types/tmp, does this not solve your problem? |
I am working on it on a different branch, the code itself is getting into a shape I am fighting with the tests. :) |
@raszi what about pushing your changes, then? Maybe we can figure out fixing the tests together? |
@paulmelnikow and why would you overgo |
It’s not that we want to contribute to @types/tmp, it’s that our types depend on it. We are shipping our own types – which is preferable because it lets us keep them up to date as our API changes – but it means we either need to add a dependency on @types/tmp which our non-TS users don’t need (there’s some reluctance to this) or require our TS users to to add their own correctly matched @types/tmp dependency. If the types rarely change it is even less ongoing effort to maintain them here once they’re pulled in. I’d like to see the promise capability added to this library. This which would be a sizable change to the types, though not hard if this package adopts the API we’ve worked out I’m happy to be part of moving tmp forward as well! Would much rather do that here in a way that supports both TS and JS users –and async! 😁 |
Before switching the whole library to TypeScript, a first step could be moving I just opened DefinitelyTyped/DefinitelyTyped#44241 to update the types to Like @paulmelnikow, I also use |
@ehmicky I am currently working on a complete rewrite of tmp using typescript. This will include the required typings, however, it is not planned to include the typings from DefinitelyTyped. |
Thanks for your answer! This works too, since that removes the need of updating separately DefinitelyTyped 👍
|
closing since the original maintainer shows no interest in this project anymore. |
@raszi while Typescript may be a valid candidate for node-tmp, it is not a candidate for refactoring the existing sources.
RATIONALE
The text was updated successfully, but these errors were encountered: