fix: action server types

[JGAntunes]

Public API Changes

Changes for the TS types for:

• ExecuteCallback for the action server
• GoalCallback for the action server

Description

Both of these callbacks seem to have wrong types setup. The first one should return a Promise which is then resolved in:

• rclnodejs/lib/action/server.js

  Lines 359 to 361 in 73f4036

  	try {
  	result = await callback(goalHandle);
  	} catch (error) {

As for the GoalCallback type as per the docs:

• "The purpose of the goal callback is to decide if a new goal should be accepted or rejected. The callback should take the goal request message as a parameter and must return a GoalResponse value. "

And based on the code:

• rclnodejs/lib/action/server.js

  Lines 259 to 261 in 73f4036

  	let result = this._goalCallback(
  	request.goal.toPlainObject(this.typedArrayEnabled)
  	);

I've tested out both of these changes in a local project and they work as expected 👍

[coveralls]

Coverage increased (+0.07%) to 90.544% when pulling 88727ec on JGAntunes:fix/action-server-types into 73f4036 on RobotWebTools:develop.

premature approval

[wayneparrott]

@JGAntunes would you also review test/test-action-server.js as I believe there is some inconsistency with executioncallback.

[fampinheiro]

@wayneparrott I reverted the type change. I expanded it to accept a callback, the callback returns the previous type or a promise that resolves for that type.

Let me know if you are okay with this change.

[wayneparrott]

@JGAntunes @fampinheiro appreciate you identifying the callback return type inconsistencies. Additionally in server.js I noticed that cancelCallback is awaited on yet the defaltCancelCallback fn does not return a promise. This leads me to believe that the original coder is attempting to account for the case where the executionCallback and cancelCallback functions could be long running async operations. Thus I believe cancelCallback definition should be modified similarly to your proposed modification of the exeutionCallback return type of Promise<ActionResult<T>> | ActionResult<T>. Something like this:

type CancelCallback = () => CancelResponse | Promise<CancelResponse>;

Let's loop @minggangw and others that may know the action api better than me for their thoughts.

[minggangw]

Thus I believe cancelCallback definition should be modified similarly to your proposed modification of the exeutionCallback return type of Promise<ActionResult> | ActionResult.

I agree, we need to change this accordingly, and it can be separated into another PR, thanks!

[minggangw]

Thanks for your PR, @wayneparrott pls help to merge it if we decide to submit another PR to resolve the comments.

[wayneparrott]

@JGAntunes again thx for identifying this issue and the suggested fix. The fix LGTM. Are you able to submit a similar fix for the cancelCallback? If no, I'll open a separate PR.

[JGAntunes]

Hey @wayneparrott 👋 I can look into it today 👍

[wayneparrott]

@JGAntunes no problem. I decided to move forward and merge this PR. I'll open a 2nd PR for the cancelCallback change.