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

Switching to Typescript #157

Closed
silkentrance opened this issue Nov 27, 2017 · 21 comments · May be fixed by #281
Closed

Switching to Typescript #157

silkentrance opened this issue Nov 27, 2017 · 21 comments · May be fixed by #281
Milestone

Comments

@silkentrance
Copy link
Collaborator

silkentrance commented Nov 27, 2017

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

RATIONALE

  • packaging and build process is far more complicated
  • dev dependencies increase a manifold
  • and believe you me 😬
@silkentrance silkentrance changed the title Swjt Discussion: Switching to Typescript Nov 27, 2017
@silkentrance
Copy link
Collaborator Author

silkentrance commented Nov 27, 2017

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
Copy link
Owner

raszi commented Nov 28, 2017

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

silkentrance commented Nov 28, 2017

@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
Copy link

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
Copy link
Collaborator Author

silkentrance commented Dec 1, 2017

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

@raszi raszi changed the title Discussion: Switching to Typescript Switching to Typescript Dec 2, 2017
@silkentrance
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

silkentrance commented Mar 20, 2019

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
Copy link
Owner

raszi commented Mar 20, 2019

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
Copy link
Collaborator Author

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

@silkentrance
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

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

@raszi
Copy link
Owner

raszi commented Oct 7, 2019

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

@silkentrance
Copy link
Collaborator Author

silkentrance commented Oct 7, 2019

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

@silkentrance
Copy link
Collaborator Author

silkentrance commented Oct 7, 2019

@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
Copy link

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

@ehmicky
Copy link

ehmicky commented Apr 26, 2020

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 DefinitelyTyped/DefinitelyTyped#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 (benjamingr/tmp-promise#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 (benjamingr/tmp-promise#42).

@silkentrance
Copy link
Collaborator Author

@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
Copy link

ehmicky commented Apr 27, 2020

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

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

@silkentrance silkentrance added this to the 0.3.0 milestone Nov 25, 2020
@silkentrance silkentrance linked a pull request Aug 29, 2022 that will close this issue
@silkentrance
Copy link
Collaborator Author

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

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

Successfully merging a pull request may close this issue.

5 participants