Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 12, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cf7f1f
Add polyfill for AbortSignal.any() for Node.js <= 18.20
minggangw Nov 11, 2025
e96119a
Address comments
minggangw Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/linux-arm64-build-and-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:
strategy:
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.
architecture: [arm64]
ros_distribution:
- humble
Expand Down
26 changes: 26 additions & 0 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,32 @@ const DistroUtils = require('./distro.js');
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.
// AbortSignal.any() was added in Node.js 20.3.0
// See https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/any_static
if (!AbortSignal.any) {
AbortSignal.any = function (signals) {
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.
if (signal.aborted) {
controller.abort(signal.reason);
break;
}

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.
}

return controller.signal;
};
}

/**
* @class - Class representing a Client in ROS
* @hideconstructor
Expand Down
6 changes: 6 additions & 0 deletions test/test-async-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,12 @@ describe('Client async functionality', function () {
});

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.
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.
if (major < 18 || (major === 18 && minor < 20)) {
this.skip();
}

const request = { a: BigInt(1), b: BigInt(1) };

try {
Expand Down
Loading