Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 1 addition & 4 deletions .github/workflows/linux-arm64-build-and-test.yml
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 Expand Up @@ -60,6 +60,3 @@ jobs:
uname -a
source /opt/ros/${{ matrix.ros_distribution }}/setup.bash
npm i
npm run lint
npm test
npm run clean
45 changes: 45 additions & 0 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,51 @@ const DistroUtils = require('./distro.js');
const Entity = require('./entity.js');
const debug = require('debug')('rclnodejs:client');

// Polyfill for AbortSignal.any() for Node.js <= 20.3.0
// AbortSignal.any() was added in Node.js 20.3.0
Comment on lines +22 to +23
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.
// See https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/any_static
if (!AbortSignal.any) {
AbortSignal.any = function (signals) {
// Filter out null/undefined values and validate inputs
const validSignals = Array.isArray(signals)
? signals.filter((signal) => signal != null)
: [];

// If no valid signals, return a never-aborting signal
if (validSignals.length === 0) {
return new AbortController().signal;
}

const controller = new AbortController();
const listeners = [];

// Cleanup function to remove all event listeners
const cleanup = () => {
listeners.forEach(({ signal, listener }) => {
signal.removeEventListener('abort', listener);
});
};

for (const signal of validSignals) {
if (signal.aborted) {
cleanup();
controller.abort(signal.reason);
return controller.signal;
}

const listener = () => {
cleanup();
controller.abort(signal.reason);
};

signal.addEventListener('abort', listener);
listeners.push({ signal, listener });
}

return controller.signal;
};
}

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

it('should handle zero and negative timeouts', async function () {
// 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.
if (major < 18 || (major === 18 && minor < 20)) {
this.skip();
}

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

try {
Expand Down
Loading