better typescript typings #551
Conversation
|
Thanks for your PR and cleaning up the code, overall looks good and some small changes need to be done I think
I am going to fix the errors due to the wrong configuration of the CI (Windows & macOS), so we can get the green badges again, thanks! |
|
CI failure has been fixed (b84455f), please rebase your branch. |
…ot have a service
should be fixed now
what we do here usually is to squash merge the PR with a suitable commit log. Should I open another PR with the commit squashed? |
It's ok that I am going to squash these commits when merging, so please offer a proper commit log that I can paste it when squashing, thanks! |
|
add generation of constants as This would allow this to work generated js will have the constant inlined |
* generate type class strings for messages and services * narrow down `TypeClass` type from `string` to known values * generate typings for *Wrapper javascript classes * generate constants as 'const enum' These provides intellisense for typeclass strings and allow constants to be referenced by value.
|
@wayneparrott any comments on this PR, I am going to move forward to merge it if it looks good. |
|
Apologies been working on the ros2cli build-type enhancement. I’ll give this PR a look tomorrow if that is not too late. |
|
@koonpeng thx for creating this PR. I had a few mins today to merge it locally and test drive. I invite @minggangw to comment as well as your thoughts. A couple of topics:
My preference is NO. Put constants close to their message and message-wrapper, not separate namespace. Here's where I'm coming from - my mental model for the interfaces.d.ts namespace structure. ROS has 3 types of communications: pub/sub, client/server and action. The namespace design of interfaces.d.ts follows this pattern Do we want to expand <pkg>/<type> such that type can include 'constants'. And the actual constants enums live in a separate namespace rather than the same namespace as the message that they apply. If NO, an alternative is to append a suffix like 'Constants' to the generated enums and have them reside in the same namespace as the msg, e.g, instead of visualization_msgs/constants/ImageMarker generate visualization_msgs/msg/ImageMarkerConstants. Thoughts?
type MessageTypeClassStr =
'string' |
'action_msgs/msg/GoalInfo' |
...
type ServiceTypeClassStr =
'action_msgs/srv/CancelGoal' |
...
type TypeClassStr = MessageTypeClassStr | ServiceTypeClassStr;Rather than the *Str naming suffix, in this case I prefer a *Name pattern resulting in: TypeClassName, MessageClassName, ServiceClassName. Since this is TypeScript, tooling will help developers to quickly determine the type is a string so no need for the 'Str'. Thoughts? |
I feel this is up for debate, I thought about several ways to do this but each has their pros and cons.
The problem is that newer version of typescript sets a limit to the depth of conditional types, if microsoft ever decides to revert the change we would be able to avoid the casting and have something exactly the same as the js code.
That's a good idea, it makes the naming more verbose. |
|
@koonpeng good feedback. Assuming we proceed with this PR's use of a 'constants' namespace, I have 3 additional thoughts.
|
|
@koonpeng this might help in the discussion. Just for fun I took a look at /opt/ros/eloquent/share/visualization_msgs/msg/ImageMarker.idl on my system. Notice the generation of the |
|
I just saw this here http://design.ros2.org/articles/interface_definition.html A quick test indeed shows that messages cannot have underscore, so a suffix of If there is no objections, I will go ahead and change the constants to use For the *Wrapper messages, I am thinking about moving them to their own modules to reflect the path of the js codes. The *Wrapper interfaces is different in that they represent actual js classes, the constants and the main definitions on the other hand does not map to any js objects. The advantage is that this allows the *Wrapper interfaces to be imported (although they are not supposed to be public in rclnodejs), and it would probably be cleaner to properly define them in the correct location. |
|
I am on the Chinese New Year holidays and just come back home 😃 By looking through the comments quickly, I think it's feasible to add the suffix for the const values. So I think we can move forward, @wayneparrott please help to merge it if both of you agree with the implementation when PR is ready. |
|
@koonpeng Thx for the PR update. I locally merged this PR and generated messages and interface.d.ts.
|
|
The wrapper are in a weird situation now, originally I wrote the wrappers to provide type information in But I got into a roadblock as typescript limits the depth of conditional types to 50. This puts the wrapper in this weird situation
This is further complicated by the fact that the recommended way to access the constants in js is through Maybe there is another way to provide type information for A simple compile test does show less performance with the wrappers, although not by a lot. with import rclnodejs without import rclnodejs |
|
I am trying to follow the current problem we meet here and have some ideas to share with you if it's useful. Now we leverage Lines 272 to 277 in d4579b2 So the overhead is to do a traversal of all messages, read them from disk and write the rclnodejs/rosidl_gen/idl_generator.js Lines 47 to 51 in d4579b2 to Line 95 in d4579b2 If it's feasible we have 2 things to ensure:
Correct me if anything is wrong, thanks! |
|
I believe @wayneparrott is talking about parsing the generated typescript definitions, the typescript maximum depth is also an issue with typescript refusing to compile the generated definitions. Combining the js and ts generation may improve performance but the problem is in the generated definitions and not the processing of generating them. For the wrappers I am just not sure how to provide any usage examples as their usage are questionable. Ideally their usage should be hidden from the user like the js code, the user should use |
Thanks for the clarification, it's clear now. I spend some time to learn the concept of Conditional Types in typescript and have a deep understanding now. I am afraid the developers may have a little bit confused about the "Wrapper" due to the explicit cast when using |
|
@koonpeng
I've wondered about this as well and have the following questions:
Later today I plan to try a few experiments using wrappers to improve my understanding of the developer experience working with the msgs and wrappers. |
|
Sorry for the late response as I was busy with other stuffs, I think the wrappers could still be useful for some edge cases. It shouldn't hurt performance too much if we move them to their own dirs. |
|
As an avid TypeScript user, I'd like to chime in here. There shouldn't be any need for complex/nested conditional types here. To get the type associated with a message you can use a type map in combination with the keyof operator + indexing, and finally add a basic conditional type to support string or object. Example (all type names are purely for example only): // Define all messages mapped to their respective type
type Messages = {
'action_msgs/msg/GoalInfo': action_msgs.msg.GoalInfo;
'action_msgs/msg/GoalStatus': action_msgs.msg.GoalStatus;
// ...
}
// Define all services mapped to their respective type
type Services = {
'action_msgs/srv/CancelGoal_Request': action_msgs.srv.CancelGoal_Request;
'action_msgs/srv/CancelGoal_Response': action_msgs.srv.CancelGoal_Response;
// ...
}
// These will contain the exact same values as before
type MessageTypeClassName = keyof Messages;
type ServiceTypeClassName = keyof Services
type Message = Messages[MessageTypeClassName] | Services[ServiceTypeClassName];
type TypeClassName = MessageTypeClassName | ServiceTypeClassName;
// Split original TypeClass to separate messages from services
type MessageTypeClass =
(() => any) |
MessageTypeClassName |
{ package: string; type: string; name: string };
type ServiceTypeClass =
(() => any) |
ServiceTypeClassName |
{ package: string; type: string; name: string };
type TypeClass = MessageTypeClass | ServiceTypeClass;
// Now define our conditional types to support message name or object
type MessageType<T extends MessageTypeClass> = T extends MessageTypeClassName ? Messages[T] : Messages[MessageTypeClassName];
type ServiceType<T extends ServiceTypeClass> = T extends ServiceTypeClassName ? Services[T] : Services[ServiceTypeClassName];
type MessageOrServiceType<T extends TypeClass> = T extends TypeClassName ? (Messages & Services)[T] : Message;We can then leverage these types in require, createMessageObject, createSubscription, createPublisher, etc by using generics. // Example usage: rclnodejs.require('sensor_msgs/msg/CompressedImage')
//
// Expands to:
// rclnodejs.require<"sensor_msgs/msg/CompressedImage">(
// name: "sensor_msgs/msg/CompressedImage"): rclnodejs.sensor_msgs.msg.CompressedImage
function require<T extends TypeClass>(name: T): MessageOrServiceType<T>;
// Example usage (with correct callback message type):
//
// this.node.createSubscription('sensor_msgs/msg/CompressedImage', ...)
function createSubscription<T extends MessageTypeClass>(typeClass: T, topic: string,
options: Options, callback: SubscriptionCallback<MessageType<T>>): Subscription;
function createPublisher<T extends MessageTypeClass>(typeClass: T, topic: string, options?: Options): Publisher;
function createMessageObject<T extends TypeClass>(type: T): MessageOrServiceType<T>where type SubscriptionCallback<T extends Messages[MessageTypeClassName]> = (
(message: T) => void
);This solution ensures the proper types without the need for any casting or deep conditional operators. The example above can be improved much further (for example you can get rid of the conditional types completely by switching to method overloads, which will simplify things), but this should at least give the general idea. |
|
In response to my last comment, I'd recommend using function overloads over conditional types to avoid unnecessary complexity. The example above is simply to show that we don't need deeply nested conditional types to achieve strong typing for require and createMessageObject. |
|
@mattrichard Thanks for the suggestion! I felt that I am missing something but im not proficient enough in typescript to find the solution. If my understanding for the function overloads is right, it will go like this? Do we have to declare each overload for |
|
The amount of overloads should be fixed so we don't have to worry about generating any additional declaration files. Taking function require<T extends TypeClassName>(name: T): (Messages & Services)[T];
function require(name: string | (() => any) | { package: string; type: string; name: string }): Message;We could also split up the union type into additional overloads if we wanted, but the only benefit of doing this is that it would allow documenting each overload separately. function require<T extends TypeClassName>(name: T): (Messages & Services)[T];
function require(name: { package: string; type: string; name: string }): Message;
function require(name: () => any): Message;
function require(name: string): Message;Either way works. If you then follow this same pattern with There's a lot more enhancements that can be made, but this is a good start. Ultimately, it's up to the maintainers to decide how far we take this. |
|
@mattrichard Thx for sharing your insights on how to more fully leverage typescript in this project. We want the typescript dimension of this project to be as proper, complete and attractive to incoming ts developers as we can make it. I took the 1st baby step by adding the initial d.ts generation. @koonpeng has been pushing forward to fix my goofed code-gen of msg constants and intro a more strongly typed api where msgs/srvs are involved. Your proposal is consistent with how we would like to see typescript declaration support expand for the project.
While the extent of project enhancements is up to our lead @minggangw it would be interesting to hear additional insights and enhancement ideas.. Please consider opening a new topic and sharing your thoughts. |
|
I do see several places now that can be improved, for example in // generic publisher
class Publisher<T> extends Entity {
publish(message: T): void;
...
}
// typed overload
createPublisher<T extends MessageTypeClassName>(typeClass: T, topic: string, options?: Options): Publisher<MessageType<T>>;
// general overload
createPublisher(typeClass: string | (() => any) | { package: string; type: string; name: string }, topic: string, options?: Options): Publisher<object>;But I would keep this PR to only focus on the context of constants and On a side node, the CI is failing due to missing dependencies, I'm not sure if it is caused by the changes on this PR. |
|
@koonpeng you're correct about making the Publisher class generic, but I agree with keeping the PR focused. I'll open an issue as @wayneparrott suggested about improving TypeScript support even further. BTW, it looks like the generated types caused a lint error. > eslint --max-warnings=0 index.js types/*.d.ts scripts lib example rosidl_gen rosidl_parser test benchmark/rclnodejs && node ./scripts/cpplint.js
C:\proj\rclnodejs\types\interfaces.d.ts
3449:1 error Line 3449 exceeds the maximum line length of 120 max-len
3450:1 error Line 3450 exceeds the maximum line length of 120 max-lenThat might be difficult to address since we have no control over package name lengths. |
|
I think it's reasonable for us to improve the functionality of typescript step by step instead of doing it in a signal commit, considering that we have discussed a lot and have a clear direction. So @wayneparrott please help to merge this PR when you think it's ready, thanks!
I think we should exclude the generated files from lint, as we don't need to keep it readable and add |
|
@koonpeng I'll test out your latest update later today. Appreciate all the work you've done to improve ts support. fyi: it's a little intimidating (but learning process) participating in the ts development with you and @mattrichard. I'll do my best to perform at a proficient level. |
|
I loaded your PR and generated msgs and types. Nice work! I like the constants and smart type support for require(). One thing you can help me with is that I don’t understand the role and use of the constant-like properties defined on wrapper types. As an example see the Log_Wrapper below. Here’s how I am using the new constant definitions: Is this the correct approach? Other than this question, I have no issues with the PR. |
|
An example usage would be const Log = rclnodejs.require('rcl_interfaces/msg/Log');
const log = new Log();
log.level = Log.DEBUG;It's actually the same code as js. There are 3 types generated now
class LogWrapper {
static readonly DEBUG: number = 1;
static readonly INFO: number = 2;
level: number;
constructor(other?: LogWrapper);
}But we can't have I noticed I should change the properties of |
|
Out of curiosity is there any particular reason for using declare module 'rclnodejs' {
namespace rcl_interfaces {
namespace msg {
export class Log {
static readonly DEBUG: 10;
static readonly INFO: 20;
static readonly WARN: 30;
static readonly ERROR: 40;
static readonly FATAL: 50;
constructor(other?: Log);
stamp: builtin_interfaces.msg.Time;
level: number;
name: string;
msg: string;
file: string;
function: string;
line: number;
}
}
}
}Then if I'm not mistaken you can use |
|
just a very small thing I notice is that putting things as a class may create invalid code. import * as rclnodejs from 'rclnodejs';
const test = new rclnodejs.rcl_interfaces.msg.Log(); // typescript compiles fine but generated js is invalid
// const test2 = new rclnodejs.rcl_interfaces.msg.Log_Wrapper(); // typescript gives errorthe code is generated in js as "use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const rclnodejs = require("rclnodejs");
const test = new rclnodejs.rcl_interfaces.msg.Log();
|
My vote is yes, discontinue generation of the constants. |
Interesting. After this PR goes in I'll take a look at things more closely. I've been mainly replying based on the comments and hardly looking at the code.
That's fine with me. |
|
I agree, @wayneparrott please help to merge and past the commit log of 985c1f7 when squashing the commits. |
… make its properties readonly
TypeClasstype fromstringto known valuesUnfortunately it's not possible to provide strong typing for interfaces like
requireandcreateMessagebecause of typescript limitations (microsoft/TypeScript#34933). This PR at least allow intellisense for type class strings, e.g.It also allows to use
rclnodejs.requirelike soThis will work also,