Infer concrete types from constructor TypeClass forms#1526
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves TypeScript type inference when using “constructor-form” ROS interfaces (e.g., values returned by rclnodejs.require(...)) so that publishers/subscriptions/services/actions can infer concrete message/request/response/goal types instead of falling back to object.
Changes:
- Extend conditional types to infer request/response and goal/feedback/result types from constructor-form service/action TypeClass values.
- Update the generated TS mapping types in
rostsd_gento recognize constructor-form types. - Add
tsdtype tests and update the TypeScript demos to illustrate string vs constructor usage.
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| types/service.d.ts | Adds constructor-form inference for service request/response message types. |
| types/node.d.ts | Expands TypeClass forms to include service/action constructor-like shapes and adds guidance for descriptor form. |
| types/action_client.d.ts | Adds constructor-form inference for action goal/feedback/result types. |
| test/types/index.test-d.ts | Adds tsd coverage for constructor-form message/service/action typing. |
| rostsd_gen/index.js | Updates generated MessageType/ServiceType/ActionType conditional mappings to support constructor-form inference. |
| demo/typescript/topics/src/subscriber.ts | Documents and demonstrates subscription typing via string vs constructor forms. |
| demo/typescript/topics/src/publisher.ts | Documents and demonstrates publisher typing via string vs constructor forms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
192
to
197
| fs.writeSync(fd, ' type ServiceTypeClassName = keyof ServicesMap;\n'); | ||
| fs.writeSync(fd, ' type Service = ServicesMap[ServiceTypeClassName];\n'); | ||
| fs.writeSync( | ||
| fd, | ||
| ' type ServiceType<T> = T extends ServiceTypeClassName ? ServicesMap[T] : object;\n\n' | ||
| ' type ServiceType<T> = T extends ServiceTypeClassName ? ServicesMap[T] : T extends { Request: any; Response: any } ? T : object;\n\n' | ||
| ); |
Comment on lines
205
to
210
| fs.writeSync(fd, ' type ActionTypeClassName = keyof ActionsMap;\n'); | ||
| fs.writeSync(fd, ' type Action = ActionsMap[ActionTypeClassName];\n'); | ||
| fs.writeSync( | ||
| fd, | ||
| ' type ActionType<T> = T extends ActionTypeClassName ? ActionsMap[T] : object;\n\n' | ||
| ' type ActionType<T> = T extends ActionTypeClassName ? ActionsMap[T] : T extends { Goal: any; Feedback: any; Result: any } ? T : object;\n\n' | ||
| ); |
|
coverage: 85.523% (+0.03%) from 85.492% — minggangw:fix-1525 into RobotWebTools:develop |
Comment on lines
193
to
197
| fs.writeSync(fd, ' type Service = ServicesMap[ServiceTypeClassName];\n'); | ||
| fs.writeSync( | ||
| fd, | ||
| ' type ServiceType<T> = T extends ServiceTypeClassName ? ServicesMap[T] : object;\n\n' | ||
| ' type ServiceType<T> = T extends ServiceTypeClassName ? ServicesMap[T] : T extends { Request: any; Response: any } ? T : object;\n\n' | ||
| ); |
Comment on lines
205
to
210
| fs.writeSync(fd, ' type ActionTypeClassName = keyof ActionsMap;\n'); | ||
| fs.writeSync(fd, ' type Action = ActionsMap[ActionTypeClassName];\n'); | ||
| fs.writeSync( | ||
| fd, | ||
| ' type ActionType<T> = T extends ActionTypeClassName ? ActionsMap[T] : object;\n\n' | ||
| ' type ActionType<T> = T extends ActionTypeClassName ? ActionsMap[T] : T extends { Goal: any; Feedback: any; Result: any } ? T : object;\n\n' | ||
| ); |
Comment on lines
+36
to
38
| "dependencies": { | ||
| "rclnodejs": "file:../../.." | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support the constructor form of
TypeClass(the value returned byrclnodejs.require(...)) with full type inference for messages, services, and actions, bringing it on par with the existing string and object-descriptor forms, and make the runtime loader accept that form so it works end to end.Types
TypeClassnow recognizes message, service ({ Request, Response }), and action ({ Goal, Feedback, Result }) constructor shapes (types/node.d.ts,types/service.d.ts,types/action_client.d.ts).ServiceRequestMessage/ServiceResponseMessageandActionGoal/ActionFeedback/ActionResultinfer the concrete payload types from the constructor.ServiceType/ActionTypeand therostsd_gengenerator (rostsd_gen/index.js) handle the constructor form.Runtime
lib/interface_loader.js:loadInterface()accepts an already-loaded interface constructor and returns it as-is (idempotent), validated via the generated class's statictype()method. Arbitrary functions still fall through to the existingMESSAGE_NOT_FOUNDerror. This fixes theMESSAGE_NOT_FOUNDfailure when a constructor was passed toActionServer/ActionClient, which callloadInterfaceunconditionally.Tests
test/types/index.test-d.ts:tsdcoverage for everyTypeClassform — message, service, and action via constructor, string, and object descriptor.test/test-action-server.js,test/test-service.js: runtime regression tests that pass the constructor form toActionServer/ActionClientandcreateService/createClientand exercise a full round-trip.test/test-message-type.js: unit tests forloadInterface()idempotency and the arbitrary-function rejection path.Demos
topics,services, andactionsTypeScript demos updated to demonstrate both the string-name and message/service/action-class styles, with typed callbacks viatypeof Ctor.file:../../..) and documentnpm install --ignore-scripts; README source examples use the current LTS (Lyrical).Fix: #1525