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:

• https://github.com/RobotWebTools/rclnodejs/blob/73f40365e2257c372db446274af63f7ac1e4f010/lib/action/server.js#L359-L361

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:

• https://github.com/RobotWebTools/rclnodejs/blob/73f40365e2257c372db446274af63f7ac1e4f010/lib/action/server.js#L259-L261

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 88727ece3b3d7622823f498af55071ba9d994332 on JGAntunes:fix/action-server-types into 73f40365e2257c372db446274af63f7ac1e4f010 on RobotWebTools:develop.

[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!

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