DevEx improvements

[lishaduck]

Hi, @jfmengels! First off, thanks for elm-review! It's super useful and I use it whenever I write Elm. It's just great.

I've been getting used to the codebase, and I had a few thoughts:

1. Use turbo instead of npm run. Turbo is a faster, parallel task runner written in Rust by the Vercel team. It'd help speed up npm run test, which is pretty slow on my machine.
2. Author .ts, not .d.ts. I respect that you don't want to write this in TypeScript, that's completely fair! However, .d.ts is a compiled output format that shouldn't be manually written, per the typescript team. I'd switch the extensions (just for the types, not the source.¹)
3. Switch to vitest—it's faster than Jest, and (as far as I can tell) supports CJS (though I've only authored ESM before so don't quote me).

I'd be happy to do all this, 1 and 2 are trivial and are directly annoying me, so I'm motivated 😄

Footnotes

1. If you're fine with bumping development Node to ^18.18.0 || >=20.6.0, tsx can run TS natively, like ts-node but better. It would make predicates format a lot nicer, but that's a pretty weak reason so I don't think it's necessary. ↩

[lishaduck]

Another thing: I hit GH rate limits bisecting tests and now I can't run them at all.

[jfmengels]

Hi @lishaduck!

Thanks for all the PRs! I'll be away at Elm Camp this week, I'll try to review them when I can, sorry if there's a bit of delay in handling them!

1. Use turbo instead of npm run. Turbo is a faster, parallel task runner written in Rust by the Vercel team. It'd help speed up npm run test, which is pretty slow on my machine.

In some projects, I use npm-run-all to run things in parallel. One downside with it is that if you do run things in parallel and one fails, which one failed can be a bit unclear. Anyway, I've added npm run test:parallel to do that. That said, there are some very slow and long tests, so they will keep taking a lot of time.

I don't know turbo, does it what npm-run-all does here, or does it bring additional good things (hopefully with low effort and buy-in).

2. Author .ts, not .d.ts. I respect that you don't want to write this in TypeScript, that's completely fair! However, .d.ts is a compiled output format that shouldn't be manually written, per the typescript team. I'd switch the extensions (just for the types, not the source.1)

That sounds good to me. The main reason why I didn't want to convert the JS source (not type) files to JS is because I don't want to bang my head in debugging only because I forgot to re-compile JS code. I hadn't thought (also, not felt the need) about writing the types in .ts files. Go ahead 👍

As for using ts-node (or tsx, hadn't heard of that one yet), I do want to make sure that I don't publish code that only works with those tools, I want to be sure that when I publish, the code still works with a plain node runner.

As for development Node version, in practice the development version now has to be >=20, because otherwise some of the tests get different results (due to JSON.parse(...) returning different pieces of information on v18 and v20. Is there a field where I can/should write that development version should be that version?
That said, I would ideally prefer that development happens at the lowest supported version, so that I easily notice breakages, I didn't do that correctly in the past (and now some supported versions don't work, as you have noticed).

3. Switch to vitest—it's faster than Jest, and (as far as I can tell) supports CJS (though I've only authored ESM before so don't quote me).

The really slow part of the tests is the run.sh test, which is a lot more convoluted and runs the CLI in a lot of different scenarios. Moving more and more of those tests to plain JS tests (or ones that don't spawn the entire thing) could make things a bit faster, but they should give the same reliability as today (or close to at least).

I am not against using vitest instead of Jest tests though, but I'm also not seeing a big reason for it (unless it is really much faster).

4. Another thing: I hit GH rate limits bisecting tests and now I can't run them at all.

You can run GITHUB_AUTH=<api-github-token> npm run test-run (same for test-run-record) to not hit the rate limits. I've written some small instructions here: https://github.com/jfmengels/node-elm-review/blob/main/test/run.sh#L13-L22

You can also put the environment variable in your .bashrc file if you'd prefer. Let me know if you run into issues with this.

[lishaduck]

Thanks for all the PRs! I'll be away at Elm Camp this week, I'll try to review them when I can, sorry if there's a bit of delay in handling them!

Have fun!

1. Use turbo instead of npm run. Turbo is a faster, parallel task runner written in Rust by the Vercel team. It'd help speed up npm run test, which is pretty slow on my machine.

In some projects, I use npm-run-all to run things in parallel. One downside with it is that if you do run things in parallel and one fails, which one failed can be a bit unclear. Anyway, I've added npm run test:parallel to do that. That said, there are some very slow and long tests, so they will keep taking a lot of time.

I don't know turbo, does it what npm-run-all does here, or does it bring additional good things (hopefully with low effort and buy-in).

It's essentially a finer-grained npm-run-all. I'd seen that npm-run-all had some compatibility issues last time I looked at it, which is why I use Turbo, but it shouldn't make a difference in a non-monorepo setting.

2. Author .ts, not .d.ts. I respect that you don't want to write this in TypeScript, that's completely fair! However, .d.ts is a compiled output format that shouldn't be manually written, per the typescript team. I'd switch the extensions (just for the types, not the source.1)

That sounds good to me. The main reason why I didn't want to convert the JS source (not type) files to JS is because I don't want to bang my head in debugging only because I forgot to re-compile JS code. I hadn't thought (also, not felt the need) about writing the types in .ts files. Go ahead 👍

I'll go ahead and switch it sometime.

As for using ts-node (or tsx, hadn't heard of that one yet), I do want to make sure that I don't publish code that only works with those tools, I want to be sure that when I publish, the code still works with a plain node runner.

Certainly! As I'd said, I don't think it's necessary, but if JSDoc got too terribly verbose, you could use a bundler for the npm tarball.

As for development Node version, in practice the development version now has to be >=20, because otherwise some of the tests get different results (due to JSON.parse(...) returning different pieces of information on v18 and v20. Is there a field where I can/should write that development version should be that version? That said, I would ideally prefer that development happens at the lowest supported version, so that I easily notice breakages, I didn't do that correctly in the past (and now some supported versions don't work, as you have noticed).

I don't think so. If JSON.parse is different, is that breaking to the rest of the cli?

3. Switch to vitest—it's faster than Jest, and (as far as I can tell) supports CJS (though I've only authored ESM before so don't quote me).

The really slow part of the tests is the run.sh test, which is a lot more convoluted and runs the CLI in a lot of different scenarios. Moving more and more of those tests to plain JS tests (or ones that don't spawn the entire thing) could make things a bit faster, but they should give the same reliability as today (or close to at least).

I am not against using vitest instead of Jest tests though, but I'm also not seeing a big reason for it (unless it is really much faster).

Ok! Maybe someday, but I'd agree that it's not worthwhile at this point. Parallelizing it should help.

4. Another thing: I hit GH rate limits bisecting tests and now I can't run them at all.

You can run GITHUB_AUTH=<api-github-token> npm run test-run (same for test-run-record) to not hit the rate limits. I've written some small instructions here: https://github.com/jfmengels/node-elm-review/blob/main/test/run.sh#L13-L22

You can also put the environment variable in your .bashrc file if you'd prefer. Let me know if you run into issues with this.

Ah. Thanks a ton! For future folks, I might put that in CONTRIBUTING.md somewhere. Then again, I don't know how common it is to run into it 🤷‍♂️

[lishaduck]

It's essentially a finer-grained npm-run-all. I'd seen that npm-run-all had some compatibility issues last time I looked at it, which is why I use Turbo, but it shouldn't make a difference in a non-monorepo setting.

I take that back. npm-run-all only works half the time for me.

[lishaduck]

A PSA:

Important

The TS compiler optionskipLibCheck is helpful as it allows ignoring errors in node_modules, which are often not written well.
However, it assumes that all .d.ts files are compiled outputs, and ignores them!
I just switched the to .ts files and got a good twenty errors.
Beware.

[lishaduck]

Another testing thing I ran into with #166 is that elm-review parses jest flags as it's own. This is a bug, and should get fixed so that we can test for memory leaks with --detectOpenHandles.

[jfmengels]

That's super weird. Great catch!

[lishaduck]

Another weird testing thing (this time in the shell script): it doesn't clear temp files if you ^C, which makes sense, but then it errors b/c they're still there.
Turbo quits other tasks once it knows the pipeline has failed, but then I have to manually remove the file: rm -rf ~/.nvm/versions/node/v16.20.2/lib/node_modules/elm-review
I don't think there's a better way to do it, just a note. Maybe try to delete them before beginning?

[lishaduck]

Moved remaining comments to #169.