chore: run tests on all supported versions of Node.js #77
Conversation
fasttime
changed the title
|
|
||
| // esmock < 2.4 supports only Node.js < 19. esmock >= 2.4 supports only Node.js >= 18.6. | ||
| // If we are running Node.js < 19, replace the esmock dependency with the legacy release. | ||
| if (process.versions.modules < 111) { |
There was a problem hiding this comment.
out of curiosity: why not using process.versions.node? what's the 111? 🤔
There was a problem hiding this comment.
111 is the ABI version number of Node.js 19, they're all listed at https://nodejs.org/en/download/releases.
process.versions.node seems more intuitive, and it should be fine to use it, although it's a string, so we'll need semver to do the comparison.
|
Thanks for the review @aladdin-add, I've added a commit to update the comments, including clarification about the 111. I'd be still open to switching to |
package.json
Outdated
| @@ -30,6 +30,7 @@ | |||
| }, | |||
| "scripts": { | |||
| "lint": "eslint . --report-unused-disable-directives", | |||
| "postinstall": "node tests/patch-esmock.js", | |||
There was a problem hiding this comment.
Does postinstall script also run when user installs this package?
There was a problem hiding this comment.
good catch! 👍
I think we need to use prepare - it won't run in that case.
There was a problem hiding this comment.
Does
postinstallscript also run when user installs this package?
I think you mean when a user runs npm install @eslint/create-config in a different package folder, or when they run npm install in a different package that includes @eslint/create-config as a dependency or devDependency. In either case: I don't know 🤷 and I don't trust the npm docs too much in this regard.
I have a package with some scripts I use for testing exactly for those cases: https://unpkg.com/@fasttime/[email protected]/package.json. Now if I run npm install @fasttime/npm-test in the directory of another package, none of the scripts seems to be run. I would assume this is the behavior we should expect from this change as well, but it's clearly possible that I'm missing something.
There was a problem hiding this comment.
I believe the scripts are run in sort of silent mode. When I try with --loglevel verbose, it looks like some of the scripts (including postinstall) are run?
$ npm install --loglevel verbose @fasttime/npm-test
npm verb cli C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js
npm info using [email protected]
npm info using [email protected]
npm verb title npm install @fasttime/npm-test
npm verb argv "install" "--loglevel" "verbose" "@fasttime/npm-test"
npm verb logfile logs-max:10 dir:C:\Users\milos\AppData\Local\npm-cache\_logs\2023-10-24T12_05_00_631Z-
npm verb logfile C:\Users\milos\AppData\Local\npm-cache\_logs\2023-10-24T12_05_00_631Z-debug-0.log
npm http fetch GET 200 https://registry.npmjs.org/@fasttime%2fnpm-test 1413ms (cache revalidated)
npm http fetch GET 200 https://registry.npmjs.org/inherits 5ms (cache hit)
npm info run @fasttime/[email protected] preinstall node_modules/@fasttime/npm-test echo *** preinstall ***
npm info run @fasttime/[email protected] preinstall { code: 0, signal: null }
npm info run @fasttime/[email protected] install node_modules/@fasttime/npm-test echo *** install ***
npm info run @fasttime/[email protected] install { code: 0, signal: null }
npm info run @fasttime/[email protected] postinstall node_modules/@fasttime/npm-test echo *** postinstall ***
npm info run @fasttime/[email protected] postinstall { code: 0, signal: null }
npm http fetch POST 200 https://registry.npmjs.org/-/npm/v1/security/advisories/bulk 222ms
added 2 packages, and audited 3 packages in 2s
found 0 vulnerabilities
npm verb exit 0
npm info ok
There was a problem hiding this comment.
Interesting, I never noticed that npm was silencing the output. In this case, yes, we need to use the prepare script instead of postinstall. Now, prepare also runs on npm pack and npm publish when no dependencies are being installed, so we will also need a check to ensure that nothing happens if esmock_legacy is missing. I'll prepare an update.
There was a problem hiding this comment.
Updated, please check again.
This PR fixes the unit tests on previous versions of Node.js (< 18). These tests are currently no longer working because of a breaking change in a dependency. See for example https://github.com/eslint/create-config/actions/runs/6609176505.
Unit tests in this package use esmock as a dev dependency.
(These is not officially specified information, I determined it by testing)
In order to run unit tests on all versions of Node.js that we need to support, I have created a postinstall script that checks the version of Node.js and replaces the esmock folder in node_modules with a legacy release on Node.js < 19.