Conversation
coliff
commented
Mar 5, 2026
coliff
commented
- npm audit fix
- Node 18 compatibility fixes
- Add an error handler to walkPath so onFinish() is called when the file walker emits an error, preventing the CLI from hanging on walk failures. Rebuilds the compiled CLI output in dist and includes related config/package updates.
- npm audit fix - Node 18 compatibility fixes - Add an error handler to walkPath so onFinish() is called when the file walker emits an error, preventing the CLI from hanging on walk failures. Rebuilds the compiled CLI output in dist and includes related config/package updates.
Deploying htmlhint with
|
| Latest commit: |
63eb88e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c504838a.htmlhint.pages.dev |
| Branch Preview URL: | https://dev-coliff-npm-audit-fix.htmlhint.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily addresses dependency vulnerabilities through npm audit fix and enhances compatibility with Node.js 18. A key bug fix is the addition of an error handler in the file walker to prevent the CLI from hanging on errors. My review focuses on improving this error handling by adding user-facing error logging instead of failing silently. The other changes, including dependency updates and test configuration adjustments, seem appropriate for the stated goals.
There was a problem hiding this comment.
Pull request overview
This PR applies npm audit fix, adjusts dependencies for Node 18 compatibility, and adds an error handler to the file walker to prevent CLI hangs. The Volta node version is also changed from 20 to 18.
Changes:
- Added an
errorevent handler onwalkPath's glob stream to ensureonFinish()is called when the file walker emits an error, preventing the CLI from hanging. - Updated dependencies (
globto v13,node-sarif-builderdowngraded to 3.2.0,rimrafto v5, and minor dev dependency bumps) for Node 18 compatibility and audit fixes. - Set Jest
maxWorkers: 1and changed the Volta pinned node version from 20.19.5 to 18.20.8.
Reviewed changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/cli/htmlhint.ts |
Added error event handler on glob stream in walkPath to call onFinish() on walk failures |
package.json |
Updated glob, node-sarif-builder, rimraf, eslint, eslint-plugin-prettier versions and Volta node version |
package-lock.json |
Lockfile regenerated to reflect dependency changes |
jest.config.js |
Added maxWorkers: 1 to run tests serially |
dist/cli/htmlhint.js |
Rebuilt compiled CLI output reflecting the source changes |
| "parse-glob": "3.0.4", | ||
| "prettier": "3.8.1", | ||
| "rimraf": "6.1.2", | ||
| "rimraf": "^5.0.0", |
There was a problem hiding this comment.
rimraf was downgraded from 6.1.2 (pinned) to ^5.0.0 (range). The resolved version 5.0.10 pulls in glob@10.5.0 which is marked as deprecated in the lockfile: "Old versions of glob are not supported, and contain widely publicized security vulnerabilities." Since this is a devDependency only used for npm run clean, consider either staying on rimraf@6.x (which uses a supported glob version) or using a different approach for the clean script (e.g., rm -rf dist or Node's built-in fs.rm).
| "rimraf": "^5.0.0", | |
| "rimraf": "6.1.2", |
| "glob": "^13.0.6", | ||
| "is-glob": "^4.0.3", | ||
| "node-sarif-builder": "^3.4.0", | ||
| "node-sarif-builder": "3.2.0", |
There was a problem hiding this comment.
node-sarif-builder was downgraded from ^3.4.0 to 3.2.0 (exact pin, no caret). While the downgrade to support Node 18 makes sense, the exact pin means you won't automatically receive patch/minor fixes. Consider using ^3.2.0 to allow compatible updates while still staying below the >=20-only versions. You'll want to verify that no API changes between 3.2.0 and 3.4.0 are being relied upon in the codebase.
| "node-sarif-builder": "3.2.0", | |
| "node-sarif-builder": "^3.2.0", |
| walk.on('error', () => { | ||
| onFinish() | ||
| }) |
There was a problem hiding this comment.
Two issues with this error handler:
-
Silent error swallowing: The error parameter is discarded, making debugging very difficult. Consider accepting the error argument and logging it (e.g.,
console.error), consistent with how other error conditions in the CLI are reported. -
Potential double
onFinish()call: Depending on the stream implementation, botherrorandendevents could fire, which would callonFinish()twice. In the caller (hintAllFiles),onFinishsetsisWalkDone = trueand callscheckAllHinted(), which could invokeonFinished()twice. Consider guarding with a flag, e.g., wrappingonFinishso it only executes once.
| }, | ||
| "volta": { | ||
| "node": "20.19.5" | ||
| "node": "18.20.8" |
There was a problem hiding this comment.
The Volta node version is changed to 18.20.8, but .nvmrc still specifies v20 and the project's coding guidelines (custom-instructions/repo/.github/copilot-instructions.md:9) state "Node v20 is used for development." If the intent is to support Node 18 as a minimum, that's handled by "engines": { "node": ">=18" }. The development tooling version (Volta/nvm) should generally remain at the recommended development version (Node 20) rather than the minimum supported version, to avoid developers unintentionally using older runtime features. If you do want to change the development Node version, .nvmrc should also be updated to match.
Release v1.9.2: update package.json and package-lock.json versions. Add a changelog entry for 1.9.2 (2026-03-05) noting a dependencies update (see #1817) and update the website index banner to announce v1.9.2.
Release v1.9.2: update package.json and package-lock.json versions. Add a changelog entry for 1.9.2 (2026-03-05) noting a dependencies update (see #1817) and update the website index banner to announce v1.9.2.
