Introduce typescript types (possibly via conversion to typescript)

TimothyJones commented 2 years ago

The original repository had types through DefinitelyTyped here.

Forking means the types won't have come through. We could:

Copy the definitely-typed types to this repo
Upload a copy to definitely-typed/commit-and-tag-version
Convert this repo to typescript

I think the smallest effort would be option 1. Long term, conversion to typescript would be ideal, because then we'd be able to merge #21, #19, #18 and #16, all of which require ESM.

joh-klein commented 1 year ago

Quick note (as you are probably aware): the types from DefinitelyTyped are pretty out of date (they are for v7.0.0)

Mitiryl commented 1 year ago

Hi @TimothyJones , any updates about this?

TimothyJones commented 1 year ago

I haven't had the chance to look at this one in detail, sorry. I think it needs some close attention. I would happily accept a PR if you have some spare capacity.

If not, even just looking at the definitely-typed types to see what needs updating would be helpful.

helmturner commented 1 year ago

@TimothyJones I've started some work on option 3. What do you think about the balance between breaking changes and correctness? Are we thinking major version upgrade? For instance, there may be some libs that need swapping for the most correct types.

The first I've encountered is yargs. There are definitely-typed typings available, but they're not great. The typings for commander are much better with the @commander/extra-typings library (maybe not the best example, since I think I can make the swap without introducing breaking changes).

Also, it looks like commander has taken the lead as the preferred CLI lib since 2021. Can you think of any objections to the swap (and associated refactoring) in this particular instance, or general guidelines for other dependency updates I might encounter?

Edit: Also, do you know what the status is on deprecating the --message option? This is the --help output: [DEPRECATED] Commit message, replaces %s with new version.\nThis option will be removed in the next major version, please use --releaseCommitMessageFormat.

TimothyJones commented 1 year ago

It's late here, so I'll be brief, but since you're actively working on this, I wanted to reply quickly.

I've started some work on option 3.

Firstly: <3 Thank you! This is amazing. If you have it on a branch somewhere, I could lend a hand / take a look etc - if you want.

In general I was hoping to do this without breaking changes - as in, changing the external behaviour or API.

I have a general philosophy of doing one thing at once, so as to keep the risk (and workload) down. However, if you think it's not possible to avoid bringing in several changes, then feel free to use your judgement.

(Aside: the original author for this project was also the author of yargs (at least, I think I have that right), so I would imagine it was chosen due to familiarity and preference)

It would be good to redo the way that options are handled, as at the moment there isn't consistency in the options available via CLI or config file. There's some context here and on the linked issues from there. I don't mind replacing yargs, but it would be great to use whatever is best for supporting fixes for those issues.

My gut feel is that it might be more practical and safer to force the typings for yargs with as, any etc (as long as we can keep that constrained to one or two files) in order to get Typescript going, and then do the refactor to commander (or whatever) after.

or general guidelines for other dependency updates I might encounter?

I have two main concerns - firstly, I'd like to prioritise maintainability - part of the reason that I'd like to bring typescript in is to reduce the risk of small changes introducing big mistakes (since this project is pretty stable, it's likely to receive more small changes than big, in general - the safer we can make those, the better).

Secondly, a big refactor to typescript is going to touch both the tests and the implementation - and changing both together is pretty risky. I think it would be best to avoid changing too much of the implementation at the same time as we touch the tests.

It would be great to have a point in the repo where we can say "here's a working version where we brought in typescript" and then one where we can say "and here's where we swapped to better dependencies for typescript" - even if there's no release between the two. I like this as the path-of-least risk, but of course there may be some reason it's not practical to do it that way.

Update: I was not brief 😂

helmturner commented 1 year ago

Haha! Thanks for the quick and thoughtful response, though!

Breaking Changes

I think you're right about trying to isolate changes to the type layer. Ultimately, may be impossible to fully type everything without run-time changes, so on the first pass I'll probably have to use some anys where there's ambiguity that a type-cast can't solve.

My approach will be to get to 100% type coverage without touching any test files, ensuring run-time semantics are consistent. I'll file issues (or a super-issue?) to track any unsafe type compromises that must be made.

Options

As for https://github.com/absolute-version/commit-and-tag-version/issues/31, that will necessarily involve breaking changes. I realized this as my initial optimism about switching to commander without breaking changes began to fade when I got to types option, which is an array of objects.

Commander doesn't account for object-typed values at all, but objects can be worked around with dot notation. Unfortunately, an array of objects is off the table without some pretty funky CLI syntax.

That said, I've already done a little playing around with the swap... would it be a good idea to close https://github.com/absolute-version/commit-and-tag-version/issues/31 with a breaking change before the TS refactor?

Roadmap

It seems to me that introducing TS to popular JS libs increases their popularity. Closing https://github.com/absolute-version/commit-and-tag-version/issues/31 means a major API change for both the CLI and the JSON config. It would make commit-and-tag-version no longer backward-compatible with scripts and config files for standard-version or any version of commit-and-tag-version prior to the breaking change.

If that's on the roadmap, I would argue that making such a change now would have less downstream impact than doing so after this fork sees wider adoption.

Edit: On second thought...

Maybe I can fix #31 without breaking changes by implementing commander with:

dot-notation for existing object-valued CLI options (and maybe a transform)
diverging syntax for not-currently-available CLI options
a transform on JSON options to normalize them to the types received by CLI args

Not 100% sure

Edit edit: Nah, not that easy.

TimothyJones commented 1 year ago

I'll file issues (or a super-issue?) to track any unsafe type compromises that must be made.

Please feel free to do so!

Commander doesn't account for object-typed values at all,

For some things, it's hard to do via a CLI without resorting to json (which is pretty unpleasant in a CLI). I don't mind if those need to be specified by config - if it's going to be clumsy to specify via CLI, then a hybrid approach would be the most ergonomic for users. One drawback of the current implementation is that it doesn't always support combining options in config and CLI (see #28)

That said, I've already done a little playing around with the swap... would it be a good idea to close https://github.com/absolute-version/commit-and-tag-version/issues/31 with a breaking change before the TS refactor?

Yes, I think this makes a great deal of sense.

It seems to me that introducing TS to popular JS libs increases their popularity.

This is another good reason :) As far as I can see from searching npm, this is the most popular fork.

Closing https://github.com/absolute-version/commit-and-tag-version/issues/31 means a major API change for both the CLI and the JSON config.

Perhaps I'm missing something, but why would it be a breaking change for the JSON config?

a transform on JSON options to normalize them to the types received by CLI args

Ah! So, I had assumed that whatever format the CLI library outputs would be irrelevant from the perspective of the rest of the code. As in, we would bring options in from (potentially) a few different sources, and populate a config object. This object would probably the same as the format for the JSON config (so there isn't need for another mental model).

This approach would also mean that only one file would have to change if we ever wanted to replace the CLI args library again - and it would keep the reading of the package.json and any version.rc files / future alternative config sources independent.

It would look something like:

// Assuming combineConfig(newConfig, currentConfig) => Config

const config = combineConfig(
  readCliConfig(),
  combineConfig(readPackageConfig(), defaultConfig)
);

It would make commit-and-tag-version no longer backward-compatible with scripts and config files for standard-version or any version of commit-and-tag-version prior to the breaking change.

I don't mind making breaking changes if there's a clear reason to do so. And, we might be able to get pretty close for most common options.

I prefer releasing minor well-documented breaking changes, over bending the API out of shape to avoid making them. I've already released breaking changes since forking, and according to the npm stats, the current major release is the most popular, so people are comfortable migrating.

Edit: Maybe I can fix [...] Edit edit: Nah, not that easy.

This is very relateable 😎

SharpSeeEr commented 1 year ago

I am a huge fan of Typescript and would love to see this get converted. Has there been any progress made? I would love to help.

helmturner commented 1 year ago

I believe we came to the conclusion that we would consider #31 to be a blocker to this (see also: #28 for context). I spun up a codespace that has some playground work, but I was hoping for a bit more feedback before just going cowboy with the API 😆

Edit: I suppose there's no reason that fringe modules can't begin conversion, I just always find it easier to start from the entrypoint and work backwards.

SharpSeeEr commented 1 year ago

I don't see #31 as a blocker. It's a little more complex, but definitely possible. I'll look into it and see what I can figure out.

helmturner commented 1 year ago

💪🏽💪🏽 If you start making some progress and want to throw up a branch I'm happy to help divide and conquer. TS would be a game changer 🙌

D4RKAR117 commented 1 year ago

Just throwing around this but I'm working on a future pr over a fork, its pretty early but anyone interested to throw suggestions that can benefit the "refactor" watch this

helmturner commented 1 year ago

I've got a start on this here #69, and I've taken fewer liberties with the project config than @D4RKAR117.

@D4RKAR117 Can I recommend trying to keep commits focused around a very specific goal? It's really hard to tell what changes you've made that actually relate directly to TypeScript conversion. It looks like a lot of project-config changes that haven't been discussed by anyone so far.

D4RKAR117 commented 1 year ago

I've got a start on this here #68, and I've taken fewer liberties with the project config than @D4RKAR117.

@D4RKAR117 Can I recommend trying to keep commits focused around a very specific goal? It's really hard to tell what changes you've made that actually relate directly to TypeScript conversion. It looks like a lot of project-config changes that haven't been discussed by anyone so far.

Oh, I didn't see #68 before. You have advanced much

I'm trying to focus to move the entire lib to esm and typescript in all front including testing suite, but without directly modifying the internal logic (dome times it's needed to, but the result is the same (I hope)), so when everything is typescript compliant can we reuse test to check if the migration its successful.

Btw everything I've made can be discussed deeply and changed or reverted if something doesn't fit, but so far it's my approach to fulfill a modern compliant typescript library.

helmturner commented 1 year ago

@D4RKAR117 I don't disagree with your goal - I just think that what constitutes a 'modern compliant typescript library' might differ from one person to the next, so it's best to stick to what is obvious.

edit: updated the name for the branch, new draft PR is #69

D4RKAR117 commented 1 year ago

@D4RKAR117 I don't disagree with your goal - I just think that what constitutes a 'modern compliant typescript library' might differ from one person to the next, so it's best to stick to what is obvious.

edit: updated the name for the branch, new draft PR is #69

Yeah absolutely, it's kinda a subjective topic to discuss, but it's always refreshing to have different approaches

TimothyJones commented 1 year ago

Thanks for the link @D4RKAR117. I took a look, and from a quick read it looks like good work. I appreciate you taking the time.

I'm a little confused about a new branch being started when there's quite a bit of discussion about the work already in progress already, both above and on the linked issues - but perhaps you didn't see it beforehand or something.

I don't want to discourage new contributions or contributors, but also having two people work on separate branches to do the same thing runs the risk of both becoming demoralised and not finishing either. For this reason (and given the extensive discussion we've had already), I'm inclined to prefer the branch that @helmturner is working on already.

However, I also see that this is your second public fork - I don't know if you're new to open source or not, and I don't want you to feel that your efforts are lost or aren't appreciated. If you would be interested, I could review your branch as if I were considering it for merge. This way you would still get the feedback that you would normally get.

D4RKAR117 commented 1 year ago

Yeah, i didn't see the branch of @helmturner before starting mine, i just use this lib extensively on my projects and saw this issue, and yeah kinda new to open source collaboration. So due to that branch, it's fixing more problems than mine (like console args parsing). i will close mine and focus the effort to help on other approaches.

Keep the good work

TimothyJones commented 1 year ago

Thanks for being so understanding!

helmturner commented 1 year ago

@D4RKAR117 I noticed that you dropped a vitest config file in there - did you ever get testing with vitest working? Currently we're hitting a build error when running the tests because it's trying to test not-yet-transpiled .ts files. Switching to vitest would be a big help, but I figured it would take a complete rewrite of the testing suite. Is there anything you found out in your refactor? If you have vitest working as a replacement for mocha, that would certainly be worth a PR!

Edit: To be fair to @D4RKAR117, I don't think I had submitted the draft PR when he started, so they would have had to find the branch on my fork (which would not be likely stumbled upon).

SharpSeeEr commented 1 year ago

~~Since we have (at least) three people working on typescript conversion, would it be simpler to have a branch created on this repository that we all can submit pull requests too?~~

~~I think that will help keep the number of conflicts low. Once we have it finished, we can merge into master.~~

Nevermind! I see the branch now. I'll merge my changes into it.

For this process, we will allow merging of broken code during this process of conversion?

helmturner commented 1 year ago

@SharpSeeEr Due to the highly integrated nature of the conversion, I think it's best to coordinate our efforts as tightly as possible. I would recommend keeping a separate fork or branch that we can compare across so that we can sound the bells if one of us starts doing conflicting or duplicate work. We should check in and review often, and merge changes from each other's branches frequently. I think broken code should only be allowed on WIP branches with a single author.

SharpSeeEr commented 1 year ago

Yes, definitely work in separate forks and branches, but if we open a PR to merge our changes into the cli-update-and-typescript-conversion-wip branch in this repo, that will actually help keep us from stepping on each others toes without having to manually do comparisons. Keep the PR's small, like converting one file, and it should work fairly smoothly.

In the end, I know you guys are the project maintainers and I am a new contributor, and I will follow your lead on how we handle all this.

helmturner commented 1 year ago

Lol, I'm a new contributor as well! 😌 Teamwork!

D4RKAR117 commented 1 year ago

@D4RKAR117 I noticed that you dropped a vitest config file in there - did you ever get testing with vitest working? Currently we're hitting a build error when running the tests because it's trying to test not-yet-transpiled .ts files. Switching to vitest would be a big help, but I figured it would take a complete rewrite of the testing suite. Is there anything you found out in your refactor? If you have vitest working as a replacement for mocha, that would certainly be worth a PR!

Edit: To be fair to @D4RKAR117, I don't think I had submitted the draft PR when he started, so they would have had to find the branch on my fork (which would not be likely stumbled upon).

Yeah i achieved many times migration from mocha to vitest due it helps to no transpile test and even test type assertions, but that requires at least top level modules/exports to be changed to esm. the problem here is that a lot of code, (i.e: updaters) code its tightly coupled to some old ways to resume modules like dynamic requires and many of the code i reviewed can be async tho. so for the test must taken in consideration tho.

Its possible that the conversion to typescript needs a deep review for some core logic that seems to be more legacy-ish to this point. Node js since v14-16+ supports many esm features like import.meta.resolve and import.meta.url that can benefit the behavior of the library.

TDLR: this library must switch to esm if we want a quality conversion to typescript

EDIT: import.meta.resolve is still flagged as experimental by Node.js, but we can get benefit from some polyfills like this to solve those gaps for module resolution strategies on esm

SharpSeeEr commented 1 year ago

Just opened a PR for this. It's pretty much a full conversion at this point. Took me all day today, but I got there. Needs some review for some parts I didn't know what to do with.

helmturner commented 1 year ago

Sorry for falling off the cliff here... life got a bit crazy for me. Still is TBH but I'm starting to feel guilty for letting this hang so long 😅

Fortunately, however, I've learned a lot since then. This YouTube short is a gem - everyone in this thread should invest the 40s to watch it.

Beyond that, however, I've identified the biggest problem we face here (and a practical solution).

The Problem

The technical debt on this code-base is real. Most of the issues we have faced (and will continue to face) stem from this being written pre-ESM and before TS became a best practice. It's pretty hard to do anything incrementally because of cascading issues that arise throughout the rafactor.

The solution

It recently dawned on me that the best way to convert any codebase to TS is to start by.... not using TS. I think JSDoc types are the way to go here (at least at first):

You can write them in JS files
They give the same auto-complete and tooltips
Errors can be highlighted without enforcement (Just don't run tsc in CI)
Errors can be enforced with the flip of a switch (Just add tsc to the test flow- no build step needed)
ChatGPT can translate TS types to JSDoc for any authors unfamiliar with it
Converting to actual TS is as easy as changing the file extension and using VSCode to auto-annotate from the JSDocs

Bonus: Comments are encouraged, and they show up in the tooltips.

I've submitted a draft PR where I've done more than half of the work on a minimal 'migration'. The commits are split such that all of the "free lunch" types are in one commit, and the types that weren't so straightforward are separate. I'll continue poking at the errors and see how far I can get, but despite the type errors it still passes tests, and it should build no problem.

helmturner commented 1 year ago

Lol @TimothyJones... couldn't help but notice you started contributing to release-please not long after your last commit-and-tag-version release. Assuming you've been using it, do you see any real advantage to CATV over Release Please?

absolute-version / commit-and-tag-version