Add polyfill for AbortSignal.any() for Node.js <= 20.3.0#1322
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a polyfill for AbortSignal.any() to support Node.js versions older than 20.3.0, where this method was introduced. The implementation enables the use of AbortSignal.any() for combining multiple abort signals in the async client functionality.
Key Changes
- Added a polyfill implementation for
AbortSignal.any()inlib/client.js - Updated test to skip zero-timeout tests on Node.js versions < 18.20 where
AbortSignal.timeout(0)throws errors - Modified CI workflow to test on Node.js 16.X for ARM64 builds to verify backward compatibility
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/client.js | Implements polyfill for AbortSignal.any() to combine multiple abort signals for older Node.js versions |
| test/test-async-client.js | Adds version check to skip zero-timeout test on Node.js < 18.20 where it's unsupported |
| .github/workflows/linux-arm64-build-and-test.yml | Changes test matrix from Node.js 24.X to 16.X to verify compatibility with older versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| signal.addEventListener( | ||
| 'abort', | ||
| () => { | ||
| controller.abort(signal.reason); | ||
| }, | ||
| { once: true } | ||
| ); |
There was a problem hiding this comment.
The polyfill has a potential memory leak. When event listeners are added to input signals, they should be removed after the controller aborts to prevent memory leaks. Consider storing references to the listeners and removing them after controller.abort() is called, or use an AbortController that can be used to clean them up.
| const Entity = require('./entity.js'); | ||
| const debug = require('debug')('rclnodejs:client'); | ||
|
|
||
| // Polyfill for AbortSignal.any() for Node.js <= 18.20 |
There was a problem hiding this comment.
The comment states "Node.js <= 18.20" but then says "AbortSignal.any() was added in Node.js 20.3.0". This is confusing. The polyfill should target Node.js versions < 20.3.0 where AbortSignal.any() is not available. Consider updating the first line to say "Node.js < 20.3.0" for consistency and clarity.
| // Polyfill for AbortSignal.any() for Node.js <= 18.20 | |
| // Polyfill for AbortSignal.any() for Node.js < 20.3.0 |
| }); | ||
|
|
||
| it('should handle zero and negative timeouts', async function () { | ||
| // Skip this test on Node.js < 18.20 where AbortSignal.timeout(0) throws RangeError |
There was a problem hiding this comment.
The comment references AbortSignal.timeout(0) throwing a RangeError in Node.js < 18.20, but this is actually about when AbortSignal.timeout() was fixed to support 0 timeout values. According to Node.js documentation, AbortSignal.timeout() was added in Node.js 17.3.0, and support for 0 timeout was fixed in 18.20.0. Consider clarifying this in the comment for accuracy.
| // Skip this test on Node.js < 18.20 where AbortSignal.timeout(0) throws RangeError | |
| // Skip this test on Node.js < 18.20.0: AbortSignal.timeout() was added in 17.3.0, but support for zero timeout values was only fixed in 18.20.0 (prior versions throw RangeError for AbortSignal.timeout(0)) |
| fail-fast: false | ||
| matrix: | ||
| node-version: [24.X] | ||
| node-version: [16.X] |
There was a problem hiding this comment.
Node.js 16.X reached end-of-life in September 2023 and is no longer receiving security updates. While testing on older versions may be valuable for backward compatibility, consider also testing on a supported version (18.X or 20.X) to ensure the polyfill works correctly on both old and new versions. The x64 workflow tests on 24.X and 25.X, creating a large version gap in test coverage.
| node-version: [16.X] | |
| node-version: [16.X, 20.X] |
| const controller = new AbortController(); | ||
|
|
||
| for (const signal of signals) { |
There was a problem hiding this comment.
The polyfill doesn't handle edge cases such as an empty signals array or null/undefined signals. Consider adding validation: if the array is empty, return AbortSignal.abort() (or create a never-aborting signal), and filter out any null/undefined values from the signals array to prevent runtime errors.
| const controller = new AbortController(); | |
| for (const signal of signals) { | |
| // Filter out null/undefined signals | |
| const validSignals = Array.isArray(signals) | |
| ? signals.filter(s => s != null) | |
| : []; | |
| if (validSignals.length === 0) { | |
| throw new TypeError('AbortSignal.any: signals array is empty'); | |
| } | |
| const controller = new AbortController(); | |
| for (const signal of validSignals) { |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Polyfill for AbortSignal.any() for Node.js <= 20.3.0 | ||
| // AbortSignal.any() was added in Node.js 20.3.0 |
There was a problem hiding this comment.
The PR title mentions 'Node.js <= 18.20' but the comment states 'Node.js <= 20.3.0'. According to Node.js documentation, AbortSignal.any() was added in Node.js 20.3.0, but the PR title's reference to 18.20 is inconsistent. Update the comment to clarify which versions actually need this polyfill, or update the PR title to match the comment.
| // Polyfill for AbortSignal.any() for Node.js <= 20.3.0 | |
| // AbortSignal.any() was added in Node.js 20.3.0 | |
| // Polyfill for AbortSignal.any() for Node.js < 20.3.0 | |
| // AbortSignal.any() was added in Node.js 20.3.0; this polyfill is needed for Node.js versions below 20.3.0 |
| // Skip this test on Node.js < 18.20.0: AbortSignal.timeout() was added in 17.3.0, | ||
| // but support for zero timeout values was only fixed in | ||
| // 18.20.0 (prior versions throw RangeError for AbortSignal.timeout(0)) | ||
| const [major, minor] = process.versions.node.split('.').map(Number); |
There was a problem hiding this comment.
The version parsing only extracts major and minor versions but doesn't handle the patch version. If process.versions.node is '18.20.1', the comparison major === 18 && minor < 20 will work, but the destructuring should explicitly acknowledge that patch (and potentially pre-release) versions exist. Consider using const [major, minor] = process.versions.node.split('.').slice(0, 2).map(Number); to make it explicit that only the first two components are used.
| const [major, minor] = process.versions.node.split('.').map(Number); | |
| const [major, minor] = process.versions.node.split('.').slice(0, 2).map(Number); |
minggangw
changed the title
This PR adds a polyfill for `AbortSignal.any()` to support Node.js versions older than 20.3.0, where this method was introduced. The implementation enables the use of `AbortSignal.any()` for combining multiple abort signals in the async client functionality. ### Key Changes - Added a polyfill implementation for `AbortSignal.any()` in `lib/client.js` - Updated test to skip zero-timeout tests on Node.js versions < 18.20 where `AbortSignal.timeout(0)` throws errors - Modified CI workflow to test on Node.js 16.X for ARM64 builds to verify backward compatibility Fix: #1321
This PR adds a polyfill for
AbortSignal.any()to support Node.js versions older than 20.3.0, where this method was introduced. The implementation enables the use ofAbortSignal.any()for combining multiple abort signals in the async client functionality.Key Changes
AbortSignal.any()inlib/client.jsAbortSignal.timeout(0)throws errorsFix: #1321