Cli update and typescript conversion wip (new branch name)

[helmturner]

This is a work in progress for a refactor that addresses the following issues: #29, #31, #44, #61

[helmturner]

see: #68 (comment)

[TimothyJones]

You can probably avoid all the // @ts-expect-error directives by making this an Object.keys(svConfig).filter(() => /* etc */)

[helmturner]

Unfortunately, I don't believe .filter()acts as a type guard. There is a library, ts-reset, that does this, but it's not recommended for libraries because it pollutes the global scope.

[TimothyJones]

I agree, let's not pollute the global scope 😅

What I mean is:

Object.keys(svConfig)
  .filter((key) => catVOnlyFeatures.includes(key))
  .forEach((key) => console.warn(
          `The "${key}" option is a feature of commit-and-tag-version, and is not supported by standard-version.${"\n"}Please move this option to the 'commit-and-tag-version' key.${"\n"}In a future version, this will throw an error.`))

[helmturner]

I understand your meaning, but what I'm saying is that - despite how one might expect .filter() and Object.keys() to work - the type of key remains as string throughout that pipeline. It is never narrowed to the union of literal strings that are keys of svConfig, which is what is needed to remove the type errors.

You aren't the only one to expect this behavior, though. I certainly did, as did many others. That's why I bring up ts-reset - it was written with the purpose of making some less intuitive behaviors act how you would expect, and I think this is one of the things it addresses (again, not advocating for it here lol).

[helmturner]

Wow - ignore me. I feel dumb now! I think I understand what you mean. I've refactored like so:

export const getMergedConfig = async (
  cwd?: string
): Promise<Partial<Config>> => {
  const searchDir = cwd ?? process.cwd();
  const pkgJson = (await import("path")).join(searchDir, "package.json");
  const legacyConf: LegacyConfig = (await import(pkgJson))["standard-version"];
  const modernConf: Config = (await import(pkgJson))["commit-and-tag-version"];

  Object.keys(legacyConf ?? {}).forEach((key) => {
    if (catVOnlyFeatures.includes(key as any)) {
      console.warn(
        `The "${key}" option is a feature of commit-and-tag-version, and is not supported by standard-version.${"\n"}Please move this option to the 'commit-and-tag-version' key.${"\n"}In a future version, this will throw an error.`
      );
    }
    if (modernConf && isin(modernConf, key as any)) {
      console.warn(
        `"standard-version"."${key}" in package.json is being overridden by "commit-and-tag-version"."${key}". in package.json`
      );
    }
  });

  const configFromFile = await getConfigFromFile();
  return { ...(legacyConf ?? {}), ...(modernConf ?? {}), ...(configFromFile ?? {}) };
};

That about right?

[TimothyJones]

I like the opinions here.

For ease of maintenance in the future, we could instead extend one of the @tsconfig base configs - https://github.com/tsconfig/bases - what do you think?

[helmturner]

Probably, I just pulled this from my own lib and made a couple of modifications lol. It's geared for maximum strictness.

Edit, on second thought... I kinda like having the config be explicitly defined in-source. Personal preference, though.

[TimothyJones]

In general I agree. The advantage of those particular ones are:

• Now you don't need to do a close read every time you pick up a new repo
• I feel more confident that it's definitely compatible with the node version we want

Yours looks fine to me though.

[helmturner]

I'll keep this comment open, but during development, I'll stick to a custom-curated tsconfig.json for ease of debugging. Once we know that the refactor and its accompanying tsconfig.json are all working as intended, we can look for a combination of base configs to extend to provide the same resulting config.

[TimothyJones]

This is good work! I've left a bunch of comments inline and enabled the build.

A nitpick about the commit names - fix and feat appear in the release notes, so at the moment correct imports in index.ts would appear in the release notes. We can avoid that with a squash merge - but I have a slight preference for non-squash merges, because then you preserve the author and history (which is nice when the commits are atomic like you've done here). I usually use chore: , refactor: and test: for non release notes changes.

[helmturner]

Eek, sorry about that! I've been using changesets in my lib until I convert to CatV, so I didn't even think about that. I'll squash my changes onto my fork's master branch to clean up the messages.

Also, replying to your suggestions 1 by 1, and I'll commit the changes! Also need to dig into the failed build, I missed a setting somewhere I guess... it's not pulling in the JSON module correctly.

[helmturner]

Fixes pushed! Let me know what you think.

EDIT: I did not fix the build error - no need to enable the build

[helmturner]

After taking a look at the build issue, it turns out the problem is that mocha is trying to run tests on the not-yet-transpiled .ts files. I can fix this by having TS transpile to a temp folder for testing, but when running the tests this way the coverage report looks pretty spotty (this could be due to a number of things that are hard to fix piecemeal). Maybe I can do something with source-maps in the testing config? I'll have to check.

Ultimately, the best fix would be to move to the vitest testing framework, which is buttery smooth for typescript projects (and even supports type-only testing). That's a big task, though. Are we okay with reduced coverage in the meantime?

[TimothyJones]

I don’t think we need to move to another test framework for mocha to run on typescript- I can take a look when I’m back at my desk

[TimothyJones]

So, you should just be able to drop in ts-mocha and it should work. Alternatively, you can add ts-node and run mocha with -r ts-node/register. You'll also need to change the .js tests to .ts, I think.

However, when I do this, I get a lot of errors, and they don't have valid stack traces. I'm not sure why this is - I think maybe something in the .tsconfig is throwing it off, maybe.

  1) config files
       reads config from package.json key 'commit-and-tag-version':
     TypeError: Cannot read properties of undefined (reading 'concat')
      at /Users/home/office/open-source/commit-and-tag-version/index.js:10:147
      at Generator.next (<anonymous>)
      at /Users/home/office/open-source/commit-and-tag-version/index.js:2:1383

Mocha normally prints proper stack traces with typescript (although, I don't really use it much so I'm a bit unclear on why it isn't doing it here).

I pushed this commit which is very rushed, just to get one fully typescript test at least executing. Possibly I did something wrong in the conversion - but it's up in case you want to have a look or cherry-pick it to get you going.

[helmturner]

After looking at your suggestion, as well as the nyc docs, I refactored everything accordingly. Still no luck - coverage shows at ~60%, and instead of green checks I get red tests. I'm not familiar enough with the testing suite here to tackle this one easily, I'm afraid. I'll try poking at it here and there, but I might also look at how practical a vitest replacement would be (if you're not completely opposed to it, of course).

I can't say testing is at the top of my priority list for the refactor - in fact, it might make more sense to do the testing refactor entirely separately so that we know the tests pass both before AND after the TS refactor.

[TimothyJones]

I'm not wild about vitest as I've found the documentation to be straight up inaccurate. But, it's not a preference worth blocking a PR over, so if you're doing the work, feel free to bring in whatever you feel comfortable with.

Also this prompted me to send them a PR fixing the problem that annoyed me.

I can't say testing is at the top of my priority list for the refactor - in fact, it might make more sense to do the testing refactor entirely separately so that we know the tests pass both before AND after the TS refactor.

Ideally we'd have the tests passing during the introduction of TS with minimal changes. Of course... that's not necessarily possible.

[TimothyJones]

I dug further in to this, and I have some bad news - it isn't the test framework at fault - the failures are real.

One problem is that replacing module.exports with export default is not the same - so some of the import statements weren't working. Fixing this got the tests for the package.json keys working (for example,
const JSON_BUMP_FILES = require('../../defaults').bumpfiles needed to become
const JSON_BUMP_FILES = require('../../defaults').default.bumpFiles - at least until that js file is migrated to typescript)

Some good news though - the most common problem is that the tests are written with programmatic require statements, which have to be replaced with await import() if we want to touch the tests as little as possible - when you do this, some of the tests just start working.

I suspect that the mocking might not work the same with typescript, so we might have to replace some of that too, potentially.

The tests for config object from files don't work any more - I had a brief poke around, and I think some of the new code isn't called - but I didn't look too closely. It might be the mocking.

[helmturner]

On the contrary, that's great news! Real failures beat the heck out of false negatives. I had wondered how much of it was due to the TS conversion being only partial, so it's good to know that's probably the most significant factor. I'll focus on just getting everything to full conversion, then double back for the tests to see what still fails. Ideally, there should be minimal test refactoring to get them passing.

I'd certainly rather not have to migrate to a whole new testing framework, so this would definitely be the preferred route. At the risk of derailing the conversation, though, I'm curious about what sort of inaccuracies you've come across in the Vitest docs? It would be nice to know of any foot-guns I might be unwittingly wielding!