search

eslint / generator-eslint

A Yeoman generator to help with ESLint development
Other
227 stars 51 forks source link

ci: don't install latest npm #168

Closed fasttime closed 10 months ago

fasttime commented 10 months ago

This PR fixes the CI test jobs.

Currently, the jobs will attempt to install the latests version of npm regardless of the installed version of Node.js. This has been problematic since the release of npm 10, because npm 10 does not support all versions of Node.js that this package supports.

~The fix is to explicitly indicate which version of npm (currently 9 or 10) should be installed along with Node.js.~ Upgrading npm doesn't seem necessary, so removing that step will fix the build.

bmish commented 10 months ago

Is there precedence for this solution? Curious if other repositories have to do this too or if there is a simpler solution.

fasttime commented 10 months ago

Other repos I know just don't bother upgrading npm, and will use whatever version comes bundled with Node.js via actions/setup-node.

bmish commented 10 months ago

I see. I don't see why we need to manually install/update npm in each of our Node version tests. If we need to install the latest npm manually at all, likely we only need to do that alongside the latest Node version, and not bother touching it for the other Node version tests. That way, we don't have to maintain this mapping of Node versions to npm versions.

fasttime commented 10 months ago

Fair point. The npm upgrade was introduced in this commit alongside other changes. At that time there were only three versions of Node.js to be tested, and the latest npm was version 7. I don't know what was the reason for manually upgrading npm, but if it's no longer necessary, I'd be happy to take it out and stick with the pre-installed version.

nzakas commented 10 months ago

Yeah, I don't think this is necessary. I'd rather update the supported Node.js versions and use the bundled npm to fix this problem. We're free to introduce breaking changes here whenever we want because this package is a CLI tool that generates code, so there are no real backcompat concerns.

fasttime commented 10 months ago

I have removed the step that was installing npm. There is already PR #167 to add Node.js 21 to the list of versions to be tested.