test: fix node 6 tests by pinning to sax 1.4.1 in old node versions#4925
test: fix node 6 tests by pinning to sax 1.4.1 in old node versions#4925
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the dependency installation logic to support installing multiple dependencies per Node version, with a focus on adding explicit sax dependency for older Node versions (4 and 6).
- Renamed
nodeToJsdomMatrixtonodeToDepsand changed structure from version string to array of dependency specifications - Modified installation logic to loop through multiple dependencies instead of a single jsdom package
- Consolidated npm spawn call to install all dependencies in a single command
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| var deps = nodeToDeps[majorNodeVersion] || ['jsdom@latest']; | ||
| var nodeInstallArgs = ['install', '--no-save']; | ||
| for (var dep of deps) { |
There was a problem hiding this comment.
The for...of loop syntax is not compatible with ES5, which this file explicitly targets (see comment on line 1-2 stating 'this file is purposefully written...in es5 syntax in order to be compatible with node 4+'). Use a traditional for loop instead: for (var i = 0; i < deps.length; i++).
| for (var dep of deps) { | |
| for (var i = 0; i < deps.length; i++) { | |
| var dep = deps[i]; |
There was a problem hiding this comment.
This seems legit - nice catch, Copilot! 🤖
There was a problem hiding this comment.
The comment is a bit misleading; it's true that we need the script to be node-4-compatible, but we don't actually need "ES5" compat, node 4 supports for-of
| 14: '21.1.2', | ||
| 16: '22.1.0', | ||
| 18: '26.1.0' | ||
| var nodeToDeps = { |
stephenmathieson
left a comment
There was a problem hiding this comment.
LGTM assuming Copilot's for loop concern is moot
Fixes errors like https://github.com/dequelabs/axe-core/actions/runs/19046409251/job/54395759156