raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
736 stars 92 forks source link

Switching to Typescript #157

Closed silkentrance closed 10 months ago

silkentrance commented 6 years ago

@raszi while Typescript may be a valid candidate for node-tmp, it is not a candidate for refactoring the existing sources.

RATIONALE

silkentrance commented 6 years ago

my humble proposal would be to provide typescript typings, e.g . *.d.ts, for the general Typescript community, e.g. /tmp/index.d.ts instead of moving everything to typescript.

raszi commented 6 years ago

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.

silkentrance commented 6 years ago

@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...

silkentrance commented 6 years ago

@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.

jmendiara commented 6 years ago

My2cents: From my own experience, making manual .d.ts files is a hard task, error prone and adds more (hard!) maintenance. For small codebases, once you decide you want to go ts is better making a full port.

Also, a gulp/grunt/* based workflow adds lots of boilerplate and devDependencies code that can be done simply using npm scripts Did you hear about https://github.com/TypeStrong/ts-node ?

silkentrance commented 6 years ago

@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.

  "scripts": {
    "doc": "jsdoc -c .jsdoc.json",
    "clean": "gulp clean",
    "dist": "npm run clean lint test build coverage && gulp dist",
    "deploy": "npm run clean lint test build coverage dist && gulp deploy",
    "test": "gulp test",
    "coverage": "gulp clean:build && gulp build && gulp clean:coverage && gulp coverage",
    "lint": "gulp lint",
    "bump": "gulp bump",
    "bump:major": "gulp bump:major",
    "bump:minor": "gulp bump:minor",
    "bump:patch": "gulp bump:patch",
    "bump:prerelease": "gulp bump:prerelease"
  },
  "devDependencies": {
    "@types/chai": "^4.0.5",
    "@types/mocha": "^2.2.44",
    "@types/node": "^8.0.53",
    "chai": "^4.1.2",
    "del": "^3.0.0",
    "gulp": "^3.9.1",
    "gulp-bump": "^2.8.0",
    "gulp-debug": "^3.1.0",
    "gulp-istanbul": "^1.1.2",
    "gulp-json-editor": "^2.2.1",
    "gulp-mocha": "^4.3.1",
    "gulp-sourcemaps": "^2.6.1",
    "gulp-tslint": "^8.1.2",
    "gulp-typescript": "^3.2.3",
    "gulp-watch": "^4.3.11",
    "istanbul": "^0.4.5",
    "mocha": "^4.0.1",
    "mocha-lcov-reporter": "^1.3.0",
    "mocha-typescript": "^1.1.12",
    "remap-istanbul": "^0.9.5",
    "ts-node": "^3.3.0",
    "tslint": "^5.8.0",
    "typescript": "^2.6.1"
  }

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 🥂 ). And with the latest updates to npm, the download of these dependencies is no longer a PITA as it had been.

silkentrance commented 6 years ago

@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.

silkentrance commented 5 years ago

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.

raszi commented 5 years ago

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.

silkentrance commented 5 years ago

Ok, well then it is TypeScript. Will you make the necessary changes?

silkentrance commented 5 years ago

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?

paulmelnikow commented 4 years ago

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!)

silkentrance commented 4 years ago

@paulmelnikow there is https://www.npmjs.com/package/@types/tmp, does this not solve your problem?

raszi commented 4 years ago

I am working on it on a different branch, the code itself is getting into a shape I am fighting with the tests. :)

silkentrance commented 4 years ago

@raszi what about pushing your changes, then? Maybe we can figure out fixing the tests together?

silkentrance commented 4 years ago

@paulmelnikow and why would you overgo @types/tmp as you can always contribute to that? Even more so as tmp does not change its API significantly in the future?

paulmelnikow commented 4 years ago

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 and types that have been validated by users.

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! :grin:

ehmicky commented 4 years ago

Before switching the whole library to TypeScript, a first step could be moving @types/tmp to an ambient file shipped with this repository.

I just opened https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44241 to update the types to 0.2.0 but as @paulmelnikow mentions, having to deal with DefinitelyTyped on each new release is not optimal.

Like @paulmelnikow, I also use node-tmp through tmp-promise and would love to see it merged upstream to node-tmp (https://github.com/benjamingr/tmp-promise/issues/36). Actually, the only reason I am updating the types is to be able to send a PR to tmp-promise to simplify increment the version of node-tmp (https://github.com/benjamingr/tmp-promise/pull/42).

silkentrance commented 4 years ago

@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. Also, we will not be merging tmp-promise as with the current rewrite, the overall API will change and we also do not require the extra methods (with...).

ehmicky commented 4 years ago

Thanks for your answer! This works too, since that removes the need of updating separately DefinitelyTyped :+1:

tmp-promise is a little off-topic since this is tracked in https://github.com/benjamingr/tmp-promise/issues/36, but adding promise support to node-tmp would be useful to most users, regardless of how it is done.

silkentrance commented 10 months ago

closing since the original maintainer shows no interest in this project anymore.