Skip to content

Conversation

@minggangw
Copy link
Member

@minggangw minggangw commented Nov 11, 2025

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

Copilot AI review requested due to automatic review settings November 11, 2025 09:12
Copilot finished reviewing on behalf of minggangw November 11, 2025 09:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() 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

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.

lib/client.js Outdated
Comment on lines 35 to 41
signal.addEventListener(
'abort',
() => {
controller.abort(signal.reason);
},
{ once: true }
);
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
lib/client.js Outdated
const Entity = require('./entity.js');
const debug = require('debug')('rclnodejs:client');

// Polyfill for AbortSignal.any() for Node.js <= 18.20
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// Polyfill for AbortSignal.any() for Node.js <= 18.20
// Polyfill for AbortSignal.any() for Node.js < 20.3.0

Copilot uses AI. Check for mistakes.
});

it('should handle zero and negative timeouts', async function () {
// Skip this test on Node.js < 18.20 where AbortSignal.timeout(0) throws RangeError
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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))

Copilot uses AI. Check for mistakes.
fail-fast: false
matrix:
node-version: [24.X]
node-version: [16.X]
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
node-version: [16.X]
node-version: [16.X, 20.X]

Copilot uses AI. Check for mistakes.
lib/client.js Outdated
Comment on lines 27 to 29
const controller = new AbortController();

for (const signal of signals) {
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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) {

Copilot uses AI. Check for mistakes.
@coveralls
Copy link

coveralls commented Nov 11, 2025

Coverage Status

coverage: 82.513% (-0.5%) from 83.023%
when pulling e96119a on minggangw:fix-1321
into bceb9a6 on RobotWebTools:develop.

@minggangw minggangw requested a review from Copilot November 12, 2025 02:06
Copilot finished reviewing on behalf of minggangw November 12, 2025 02:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +22 to +23
// Polyfill for AbortSignal.any() for Node.js <= 20.3.0
// AbortSignal.any() was added in Node.js 20.3.0
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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

Copilot uses AI. Check for mistakes.
// 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);
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
const [major, minor] = process.versions.node.split('.').map(Number);
const [major, minor] = process.versions.node.split('.').slice(0, 2).map(Number);

Copilot uses AI. Check for mistakes.
@minggangw minggangw changed the title Add polyfill for AbortSignal.any() for Node.js <= 18.20 Add polyfill for AbortSignal.any() for Node.js <= 20.3.0 Nov 12, 2025
@minggangw minggangw merged commit 3e58c4b into RobotWebTools:develop Nov 12, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add polyfill for AbortSignal.any() for Node.js <= 20.3.0

2 participants