-
Notifications
You must be signed in to change notification settings - Fork 159
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: automatically test on all LTS node versions #134
base: main
Are you sure you want to change the base?
Conversation
2220f66
to
daefa50
Compare
- with example for testing on extra versions - add node 21, as a "not LTS" example - ci: test windows too, for curiousity - ci: add coverage workflow - package.json: replace devDeps with npx
Is there anything I can do to nudge this along? |
package.json
Outdated
"codecov": "^3.8.3", | ||
"nyc": "^15.1.0", | ||
"mocha": "^10.2.0" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these changes, you can no longer git clone
+ npm install
to get everything you need to work offline. Please revert these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. Done.
Food for thought:
- the codecov module is officially deprecated
- nyc is almost unsupported (last release on npm, 4 years ago).
- nyc only supports es6 via babel transpilation to es5 😬
- the bare-bones support is has received is from bcoe, the author of c8, which remains the future of JS coverage reporting, using node's built-in coverage features.
- The GHA workflow uses nyc, for consistency
- the big coverage reporters (codecov, coveralls, etc.) are nudging everyone to move to GHA style processing
- the included GitHub action will report the coverage results in PRs
- moving the code coverage entirely to a GitHub Action improves the default install (
npm -i
), because 99.9% 1 of the time, nobody benefits from installing dev modules they'll never use.npm i
output now is 28 MB:added 237 packages, and audited 238 packages
- w/o nyc & codecov:
6.4M ./node_modules
- w/o nyc, codecov, & mocha:
0M ./node_modules
- I don't know about you but many of my PRs come from folks that still greatly struggle with git, let alone running
npm test
. - if
mocha
is installed globally on your machine (as may be the case for a developer who prefers it), runningnpx mocha
will use the globally installed (vianpm i -g
) version. Not only does this work great offline, but it saves a lot of wasted developer time when doing installs and reinstalls of modules. - (aside: mocha is my favorite test runner), removing mocha as a dep pairs nicely with test: replace mocha with node.js builtin #135
- Made up statistic, but lets be honest: so few people used
npm i --production
that all 68 of us noticed when npm changed it tonpm i --omit=dev
npx
I'm happy to amend this PR in any way that suits the maintainer(s).
fixes #119
Previews
--