Skip to content
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.

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

Add status field to action result messages #796

Merged
merged 8 commits into from
Nov 25, 2024
Merged

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented Sep 29, 2024

Public API Changes
Adds a new Action.setCanceled() methods to properly cancel actions server-side using roslibjs.

Description
This goes along with RobotWebTools/rosbridge_suite#953, in which a status field was added to the action_result message.

It also adds a setCanceled() helper function to the Action class to wrap the setting of the right status in cancellation settings.

Copy link
Contributor

@EzraBrooks EzraBrooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm like half asleep and on a plane right now so maybe I'm missing context, but I'm not sure I grok what's going on here. Is this an action-server-side feature or action-client-side?

@sea-bass
Copy link
Contributor Author

sea-bass commented Oct 1, 2024

I'm like half asleep and on a plane right now so maybe I'm missing context, but I'm not sure I grok what's going on here. Is this an action-server-side feature or action-client-side?

This is a server-side feature.

For context, take a look at RobotWebTools/rosbridge_suite#920 and the PR that tries to solve it (RobotWebTools/rosbridge_suite#953).

Basically, there are 2 new things:

  1. Now the action_result message carries a status with it (hence the additions to the setSucceeded()/setFailed() functions here)
  2. Also, when you cancel a goal client-side, instead of the client automatically aborting the goal, the server is responsible for telling you that it was canceled (hence the addition of the setCanceled() function here).

@sea-bass
Copy link
Contributor Author

I fully dropped the ball on re-requesting review. Now that rosbridge 2.x is released, this is needed for action support to be compatible.

@@ -216,6 +216,25 @@ export default class Action extends EventEmitter {
id: id,
action: this.name,
values: result,
status: 4, // Corresponds to GoalStatus.STATUS_SUCCEEDED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extract GoalStatus to an enum, since we've got typescript?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently since these are .js files, things are not happy if I try it directly:

src/core/Action.js:14:6 - error TS8006: 'enum' declarations can only be used in TypeScript files.

Is there some easy way around this that fits within this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this... please let me know if there's anything better.

https://stackoverflow.com/a/44447975

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try adding a typescript file for the enum and importing it into the JS file? I don't know in practice how that works, but it should transpile into an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeaaaa, that did work! Just had to add one more compiler option. Thank you.

tsconfig.json Outdated
Comment on lines 8 to 9
"allowImportingTsExtensions": true /* Enable importing TypeScript files. */,
"emitDeclarationOnly": true /* Needed to allow importing TypeScript files. */,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure these two are needed? I thought allowImportingTsExtensions just lets you put .ts in the filename instead of excluding the extension from the import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the first one, my npm run build failed.

The second one was simply suggested by my linter in VSCode but did not seem necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you mean that I could omit the extension from the import and it may work? I will try when I get a chance

Copy link
Contributor Author

@sea-bass sea-bass Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted this and have an alternate error:

Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'.
Did you mean '../core/GoalStatus.js'?ts(2835)

Indeed, the tsconfig.json has "moduleResolution": "nodenext" specified.

I could get around this by switching the settings from

    "module": "nodenext",
    "moduleResolution": "nodenext",

to

    "module": "esnext",
    "moduleResolution": "bundler",

... but to be honest with you, I have no idea what I'm touching at this point. Maybe there are undesired consequences.

Copy link
Contributor

@EzraBrooks EzraBrooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think now that we're building roslibjs with Vite, nodenext and bundler actually are the right settings for the tsconfig. I seem to recall the previous settings being a side effect of the odd halfway step we were in a year or so ago before we adopted a fully modern toolchain.

@sea-bass
Copy link
Contributor Author

I think now that we're building roslibjs with Vite, nodenext and bundler actually are the right settings for the tsconfig. I seem to recall the previous settings being a side effect of the odd halfway step we were in a year or so ago before we adopted a fully modern toolchain.

Confirming since right now this changes to esnext and bundler -- as nodenext did not allow importing without extensions.

@EzraBrooks
Copy link
Contributor

EzraBrooks commented Nov 25, 2024

sorry yea I meant esnext. I am half-braindead because yesterday I drove from Colorado to the east coast

@sea-bass sea-bass merged commit 7519937 into develop Nov 25, 2024
4 checks passed
@sea-bass sea-bass deleted the add-action-status branch November 25, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants