Uh oh!
There was an error while loading. Please reload this page.
ci: run build and test on newer node version, lint only on 12 - #19
Conversation
legobeat
commented
Nov 21, 2023
- ci: build and test on newer node version, lint only on 12
- ci: pin to npm v6 for lockfile consistency
- ci: pin to npm v6 for lockfile consistency
ab1044d to
0236012Compare
NEllusion
left a comment
There was a problem hiding this comment.
Added some initial comments to get additional context. Looking good overall!
| strategy: | ||
| matrix: | ||
| node-version: [8.x, 10.x, 12.x] | ||
| node-version: [8.x, 10.x, 12.x, 14.x, 16.x, 18.x, 20.x] |
There was a problem hiding this comment.
Do we intend to still support as far back as node 8.x, or are we continuing this for now until we are certain that we do not have any projects running node 8 which may use this?
There was a problem hiding this comment.
Adding ref here that engine is currently specified as 8 which is a factor:
Line 134 in bba33bd
There was a problem hiding this comment.
In the ethjs family of packages, have been maintaining a conservative compatibility-stance, breaking runtime (node, browsers) and package manager (npm) compatibility only on a per-need basis to address breaking bugs and regressions but now that we have working releases out (and some time has passed), I think it's about time to start removing support for older node versions. This should also open the door to reducing or entirely remove babelification.
That being said, I think would be good to not quite yet remove support for node v10 (considering it's the latest available in distro repos for Ubuntu 20.04 LTS and Debian Buster). We'll have to drop v8 to be able to upgrade to webpack v5, though, so I guess that'll be next.
There was a problem hiding this comment.
| strategy: | ||
| matrix: | ||
| node-version: [8.x, 10.x, 12.x] | ||
| node-version: [12.x] |
There was a problem hiding this comment.
How come we do not want to lint on other versions?
There was a problem hiding this comment.
While we do want to test on each supported major to ensure runtime compatibility (and therefore need test and build to work across them as well), linting should only be necessary to run once. This also means we can use newer versions of eslint.
| - run: npm i -g npm@6 | ||
| - run: npm i |
There was a problem hiding this comment.
How did you decide that npm@6 was better to use here than the default?
There was a problem hiding this comment.
I see it is the minimum version in our package.json
Line 135 in bba33bd
There was a problem hiding this comment.
Pinning npm version in order to ensure lockfile consistency.
v6 is the most recent that still works on Node.js v8.
mcmire
left a comment
There was a problem hiding this comment.
I think I know the answer to these questions, but curious of them just in case there's something new here I haven't thought about. Will give an approval in the meantime!