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

ci(nodejs): only run on LTS version #128

Merged
merged 1 commit into from
Mar 3, 2024
Merged

Conversation

EdJoPaTo
Copy link
Contributor

Node.js 20 will be a LTS version from 2023-10-24. Its probably a good idea to also test against it.

@maxlath
Copy link
Owner

maxlath commented Jul 31, 2023

That would make sense. I'm still not convinced by the CI checks though: they run automatically at times when I know they won't detect any issue, for instance after a simple rebase on the main branch. In that kind of case, they just slow down my workflow. Wouldn't there be a way to turn off the CI tests and run equivalent checks during the prepack script?

@EdJoPaTo
Copy link
Contributor Author

They basically run npm test and npm repack for the 4 versions.

Maybe their benefit is mainly on Pull Requests then for you? To ensure code additions are working and are compatible with other Node.js versions?

@maxlath
Copy link
Owner

maxlath commented Jul 31, 2023

Ensuring code additions are working is already covered by the prepack script being run before npm publish. Compatibility with other Node.js versions is harder to test this way. I was considering running tests locally in a Docker container with the desired NodeJS version, but unfortunately the Docker Hub only has node >= v16.

@EdJoPaTo
Copy link
Contributor Author

The CI basically also just installs other Node.js versions via nvm install 14.

This could be added to local scripts but I think it overcomplicates things. The main thing to ensure is the target version of typescript. Then everything else should just work as TypeScript should handle that (or fail to compile on any Node.js version). Therefore the different Node.js versions are basically a pointless test.
So the only thing to do is running CI for the current LTS version or something like that to know the contribution works with TypeScript?

@maxlath
Copy link
Owner

maxlath commented Jul 31, 2023

So the only thing to do is running CI for the current LTS version or something like that to know the contribution works with TypeScript?

Isn't that already covered by npm publish automatically running the prepack script? Which would be the worst case: I usually fetch the PRs branch and run the tests locally.

@EdJoPaTo
Copy link
Contributor Author

EdJoPaTo commented Aug 2, 2023

yeah, the CI runs npm test and npm pack like locally so it will find the same mistakes like you would locally. Also yeah, there also is a git commit hook (which I personally find a bit annoying as it eats up a lot of time because I test before I commit or have unstaged changes so the test can not ensure a working commit anyway). So there are already multiple ways of trying to ensure working code.

I do like to have a CI setup which ensures "yes, everything is working as expected" which is not some "works on my machine".

Also relevant: Typescript v5 requires Node.js 14 to compile (the result can still be used on older versions). So these tests here wont work on Node.js 12 as TypeScript cant compile the code.

I think a good way forward is using lts/* as the Node.js version the CI uses and ignore the other versions.

@EdJoPaTo EdJoPaTo force-pushed the ci-nodejs-20 branch 2 times, most recently from 1d4f08b to aa7c38f Compare September 12, 2023 01:49
@EdJoPaTo EdJoPaTo changed the title ci(nodejs): test Node.js 20 ci(nodejs): only run on LTS version Sep 12, 2023
@maxlath maxlath merged commit 08e306e into maxlath:main Mar 3, 2024
1 check passed
@EdJoPaTo EdJoPaTo deleted the ci-nodejs-20 branch March 3, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants