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

feat: switch to TypeScript #281

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from
Draft

feat: switch to TypeScript #281

wants to merge 27 commits into from

Conversation

raszi
Copy link
Owner

@raszi raszi commented Aug 28, 2022

Breaking change

This PR is about switching from JavaScript to TypeScript and changing the API at the same time with re-implementing the whole codebase instead of just "rewriting" the previous old JavaScript code in TypeScript.

The main interface is going to be more or less the same with the following caveats:

  • We won't provide a feature to remove the files "automatically"
  • We won't give a FileHandle just a path of the created file or directory

With this change, we are also dropping the support for old Node versions which are not maintained anymore according to the Node LTS release schedule: https://nodejs.org/en/about/releases/

@raszi raszi added the breaking a contract/behaviour breaking change label Aug 28, 2022
Copy link
Collaborator

@silkentrance silkentrance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise everything looks clean and nice.

  • New (usage) documentation is missing
  • Version bump to 0.3.0 ? is missing

@raszi raszi marked this pull request as draft August 29, 2022 13:21
@raszi raszi changed the title [WIP] feat: switch to TypeScript feat: switch to TypeScript Aug 29, 2022
@silkentrance
Copy link
Collaborator

What I find problematic, though, is the complete absence of the callback based interface.
Prior to completely removing it we should first deprecate it, say in an interim 0.3.0 release, still using the old code.

The new version can then be released as e.g. 0.4.0 or 1.0.0 even, with the callback interface completely gone.

Also, not having the automatic cleanup (unsafe cleanup, graceful cleanup) might alienate some users and break a lot of applications and tests. So we should first deprecated these options and then remove them.

What do you think?

@raszi
Copy link
Owner Author

raszi commented Aug 29, 2022

Code-wise everything looks clean and nice.

* New (usage) documentation is missing

* Version bump to 0.3.0 ? is missing

Thank you! Yes, all of these are missing, because this is still a work in progress version, I've added the [WIP] prefix, but changing this explicitly to draft would have been better. I did that now.

@raszi
Copy link
Owner Author

raszi commented Aug 29, 2022

What I find problematic, though, is the complete absence of the callback based interface. Prior to completely removing it we should first deprecate it, say in an interim 0.3.0 release, still using the old code.

I am planning to add that back as an optional argument. If somebody passes a callback function then we would not return a Promise, if they don't then the returned value is going to be a Promise.

The new version can then be released as e.g. 0.4.0 or 1.0.0 even, with the callback interface completely gone.

We can definitely discuss the versioning. I don't have a strong opinion about them.

Also, not having the automatic cleanup (unsafe cleanup, graceful cleanup) might alienate some users and break a lot of applications and tests. So we should first deprecated these options and then remove them.

I don't think we can deprecate them in a nice way. Since if we would provide the functionality the same way then we make this type of refactoring and cleanup impossible. I am thinking about alternatives, but I am not sure we can do too much. BTW the cleanup is technically nothing but calling an unlink() or rimraf() when it is required. Although this when is a hard question that only the application developer could decide.

@silkentrance
Copy link
Collaborator

What I find problematic, though, is the complete absence of the callback based interface. Prior to completely removing it we should first deprecate it, say in an interim 0.3.0 release, still using the old code.

I am planning to add that back as an optional argument. If somebody passes a callback function then we would not return a Promise, if they don't then the returned value is going to be a Promise.

Great.

The new version can then be released as e.g. 0.4.0 or 1.0.0 even, with the callback interface completely gone.

We can definitely discuss the versioning. I don't have a strong opinion about them.

Same here. Whatever fits the semver scheme works for me.

Also, not having the automatic cleanup (unsafe cleanup, graceful cleanup) might alienate some users and break a lot of applications and tests. So we should first deprecated these options and then remove them.

I don't think we can deprecate them in a nice way. Since if we would provide the functionality the same way then we make this type of refactoring and cleanup impossible. I am thinking about alternatives, but I am not sure we can do too much. BTW the cleanup is technically nothing but calling an unlink() or rimraf() when it is required. Although this when is a hard question that only the application developer could decide.

You could memoize the handed out temporary paths, similar to https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbageCollector.ts, https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbageDisposer.ts, and, https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbagePruner.ts.

@raszi
Copy link
Owner Author

raszi commented Aug 29, 2022

You could memoize the handed out temporary paths, similar to https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbageCollector.ts, https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbageDisposer.ts, and, https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbagePruner.ts.

I can memoize them, but when can we delete them? There is no good answer to that, I can provide a function that does the garbage collection on all the items, but is that the right solution? What if they want to do partial removal?

@silkentrance
Copy link
Collaborator

You could memoize the handed out temporary paths, similar to https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbageCollector.ts, https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbageDisposer.ts, and, https://github.com/raszi/node-tmp/blob/refactor/src/internal/GarbagePruner.ts.

I can memoize them, but when can we delete them? There is no good answer to that, I can provide a function that does the garbage collection on all the items, but is that the right solution? What if they want to do partial removal?

We had the keep and unsafeCleanup options in the past. So maybe keeping track of this also, in form of a keepIfNotEmpty for dirs and keep for files.

@raszi
Copy link
Owner Author

raszi commented Aug 29, 2022

We had the keep and unsafeCleanup options in the past. So maybe keeping track of this also, in form of a keepIfNotEmpty for dirs and keep for files.

Okay, let me think about this.

Thank you for your feedback!

@silkentrance
Copy link
Collaborator

We had the keep and unsafeCleanup options in the past. So maybe keeping track of this also, in form of a keepIfNotEmpty for dirs and keep for files.

Okay, let me think about this.

Thank you for your feedback!

Coming to think of it, why don't just make this a RetentionPolicy enum, e.g. keep: RetentionPolicy, with

enum RetentionPolicy {
  KEEP,
  DISCARD
}

where KEEP would always keep the tmp dir or file and DISCARD will always remove the dir or file.

Where unsafeCleanup would then be mapped to DISCARD unless keep == true, which would then override the unsafeCleanup option.

And where keep == false would then also be mapped to DISCARD.

@raszi
Copy link
Owner Author

raszi commented Aug 29, 2022

Well, anything but an enum! 😆

Enums cannot be mapped back to JS that was a mistake that the TypeScript developers did there.

@silkentrance
Copy link
Collaborator

silkentrance commented Aug 29, 2022

Well, anything but an enum! 😆

Enums cannot be mapped back to JS that was a mistake that the TypeScript developers did there.

Hm, enums will be transpiled into an object

export enum Foo {
        BAR,
        CAR
}

And they can be used directly from javascript...

> e = require('./enum').Foo
{ '0': 'BAR', '1': 'CAR', BAR: 0, CAR: 1 }
> e.BAR
0

@raszi
Copy link
Owner Author

raszi commented Aug 29, 2022

Loosely transpiled to JS. There are a lot of issues with that with no huge benefits. I would rather use union types with string.

@silkentrance
Copy link
Collaborator

Is it possible to leverage the use of the interface then to just string literal constants, e.g. KEEP vs. 'KEEP'?
E.g.

const KEEP = 'KEEP'
const DISCARD = 'DISCARD'
type RetentionPolicy = KEEP | DISCARD;

?

@silkentrance
Copy link
Collaborator

And I am not going into the ADAPT-ADOPT-and-IMPROVE of the information provided in the links you provided. These devs should get their gears straight. 😄

With this change we both support Promise usage and callback usage.
@raszi
Copy link
Owner Author

raszi commented Oct 31, 2022

@silkentrance I've implemented the optional callback. If that is provided then we don't return a Promise<string> but we call the callback function in an old-school way.

If the optional callback function is not passed then we'll return a Promise<string> as one would expect.

In the implementation, I've used function overloadings, and if the callback option is used then the return type is Promise<void>, therefore TS could help developers with the right types depending on the passed arguments.

@mbargiel mbargiel mentioned this pull request Jan 31, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking a contract/behaviour breaking change
Projects
None yet
2 participants