Skip to content

Commit 602f5a0

Browse files
committed
Address comments
1 parent 6b6bf4b commit 602f5a0

3 files changed

Lines changed: 53 additions & 2 deletions

File tree

lib/node.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,10 +259,11 @@ class Node extends rclnodejs.ShadowNode {
259259
let timerInfo;
260260
if (typeof rclnodejs.callTimerWithInfo === 'function') {
261261
timerInfo = rclnodejs.callTimerWithInfo(timer.handle);
262+
timer.callback(timerInfo);
262263
} else {
263264
rclnodejs.callTimer(timer.handle);
265+
timer.callback();
264266
}
265-
timer.callback(timerInfo);
266267
}
267268
});
268269

src/rcl_timer_bindings.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,12 @@ Napi::Value CreateTimer(const Napi::CallbackInfo& info) {
7979
bool lossless;
8080
int64_t period_nsec = info[2].As<Napi::BigInt>().Int64Value(&lossless);
8181
bool autostart = true;
82-
if (info.Length() > 3 && info[3].IsBoolean()) {
82+
if (info.Length() > 3) {
83+
if (!info[3].IsBoolean()) {
84+
Napi::TypeError::New(env, "Timer autostart must be a boolean")
85+
.ThrowAsJavaScriptException();
86+
return env.Undefined();
87+
}
8388
autostart = info[3].As<Napi::Boolean>().Value();
8489
}
8590
rcl_timer_t* timer =

test/test-timer.js

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
const assert = require('assert');
1818
const rclnodejs = require('../index.js');
19+
const rclnodejsNative = require('../lib/native_loader.js');
1920
const DistroUtils = require('../lib/distro.js');
2021
const sinon = require('sinon');
2122

@@ -222,6 +223,39 @@ describe('rclnodejs Timer class testing', function () {
222223
rclnodejs.spin(node);
223224
});
224225

226+
it('timer callback preserves zero arguments when TimerInfo is unavailable', function (done) {
227+
const originalFunc = rclnodejsNative.callTimerWithInfo;
228+
229+
try {
230+
Object.defineProperty(rclnodejsNative, 'callTimerWithInfo', {
231+
value: undefined,
232+
configurable: true,
233+
writable: true,
234+
});
235+
} catch (e) {
236+
this.skip();
237+
return;
238+
}
239+
240+
const timer = node.createTimer(TIMER_INTERVAL, function () {
241+
try {
242+
assert.strictEqual(arguments.length, 0);
243+
timer.cancel();
244+
done();
245+
} finally {
246+
try {
247+
Object.defineProperty(rclnodejsNative, 'callTimerWithInfo', {
248+
value: originalFunc,
249+
configurable: true,
250+
writable: true,
251+
});
252+
} catch (e) {}
253+
}
254+
});
255+
256+
rclnodejs.spin(node);
257+
});
258+
225259
it('timer.setOnResetCallback', function (done) {
226260
if (DistroUtils.getDistroId() <= DistroUtils.getDistroId('humble')) {
227261
this.skip();
@@ -382,4 +416,15 @@ describe('rclnodejs Timer class coverage testing', function () {
382416
node.createTimer(TIMER_INTERVAL, () => {}, { autostart: 'false' });
383417
}, /options\.autostart/);
384418
});
419+
420+
it('native createTimer validates autostart argument type', function () {
421+
assert.throws(() => {
422+
rclnodejsNative.createTimer(
423+
node.getClock().handle,
424+
node.context.handle,
425+
TIMER_INTERVAL,
426+
'false'
427+
);
428+
}, /Timer autostart must be a boolean/);
429+
});
385430
});

0 commit comments

Comments
 (0)