RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
319 stars 70 forks source link

Improves typing support for clients and services #709

Closed wayneparrott closed 3 years ago

wayneparrott commented 3 years ago

Improve client and service typing support.

Key changes

Fix #708

wayneparrott commented 3 years ago

Fixing the build error asap. I did not have test_interface_files pkg installed in my dev env.

update-1: At 1st glance, file generation from the generate_message is not generating the service msgs (_Request and _Response) for any of the test_interface_files/srv/*.srv files. The test_interface_files pkg has not been updated for over 13+ months. The test_msgs pkg appears to be the replacement for test_interface_files has been updated as recently as yesterday and is processed with no issues by the generate_messages script.

I plan to look into the generate_messages issue with test_interface_files pkg but would like to ask if we need to test against this package since test_msgs pkg seems to be the one we should focus on in testing?

update-2: Upon inspecting the diffs between test_interface_files pkg and the test_msgs pkg, the services in the test_interface_files pkg do not include the _Request.msg and _Response.msg files. The test_msgs pkg includes these missing files. I believe the root issue is the test_interface_files pkg is not correctly configured in its package.xml. Conclusion: the test_interface_files pkg is goofed and tsd_gen script should do a better job at identifying goofed up interfaces.

minggangw commented 3 years ago

Thanks for making the improvements for ts, I investigated the error reported on trybot

Caught error: Error: Cannot find module './test_interface_files__srv__Arrays_Request.js'
Require stack:
- C:\proj\rclnodejs\generated\test_interface_files\test_interface_files__srv__Arrays.js
- C:\proj\rclnodejs\lib\interface_loader.js
- C:\proj\rclnodejs\rostsd_gen\index.js
- C:\proj\rclnodejs\scripts\generate_messages.js
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@~2.1.2 (node_modules\chokidar\node_modules\fsevents):

Just as you have mentioned in update-2 that the services in the test_interface_files pkg don't include the _Request.msg and _Response.msg files, so the idl_generator doesn't generate the corresponding JS files for them. Because the test_interface_files has been deprecated, I think that's the reason why there is no _Request/Response.msg files, unfortunately, the .srv is still there.

Some comments based on your latest update

Conclusion: the test_interface_files pkg is goofed and tsd_gen script should do a better job at identifying goofed up interfaces.

Strongly agree, probably we could just check if the _Request/Response.msg exist practically.

wayneparrott commented 3 years ago

Added checks for actions and services to verify that the required interfaces are present before generating the respective tsd info. When an invalid service or action is detected (e.g., test_interface_files/srv/*) an informative error msg is output to stdout and the invalid interface is not included in the interfaces.d.ts file.

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 91.183% when pulling 0fc7070b0f6454070f4a6d2a7946a62fa21944b7 on wayneparrott:service-better-typing into 8823815d9908d616750f19e35c74a1e39c316384 on RobotWebTools:develop.

minggangw commented 3 years ago

After landing this PR, I think v0.16.0 is ready to publish, thanks for your effort!