-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Typescript Generic Types #4455
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
base: main
Are you sure you want to change the base?
Typescript Generic Types #4455
Conversation
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
|
@rouzwelt new maintainer here, this is an interesting direction, I would like to review it more carefully soon. It would be great if you could be clear about the limitations of the approach as well. |
hey there, great to have you on |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
|
Please stop pinging maintainers repeatedly. Your PR hasn't been forgotten, we just don't have the necessary bandwidth. FWIW we will be discussing generic support next week and try to come up with a design we want to support. Your PR will definitely help in that discussion so it is much appreciated! |
guybedford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience on this review, I finally was able to take a brief look at this.
I really like the work here and would love to see this landed, we just have to be very methodical about the approach and review process. I'm up for it though if you are.
| #[derive(Serialize, Deserialize)] | ||
| pub struct SomeType { | ||
| pub prop: String, | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize)] | ||
| pub struct SomeGenericType<T> { | ||
| pub field: T, | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize)] | ||
| pub struct OtherGenericType<T, E> { | ||
| pub field1: T, | ||
| pub field2: E, | ||
| } | ||
|
|
||
| // some error type | ||
| pub struct Error; | ||
|
|
||
| #[wasm_bindgen] | ||
| pub struct TestStruct; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All types here seem to be exported types. Can we extend the coverage here to imported types as well?
We would also need to cover interactions with js_name and js_namespace renaming to ensure aliasing is supported.
| inform(FUNCTION); | ||
| inform(0); | ||
| inform(#nargs); | ||
| #describe_args | ||
| #describe_ret | ||
| inform(#TYPE_DESCRIBE_MAP); | ||
| #ret_ty_map | ||
| inform(#TYPE_DESCRIBE_MAP); | ||
| #args_ty_map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow why we need two separate serializations here. The first-level cut-off of the generics is the function type, so we should just fold the data into describe_args and describe_ret instead of duplicating here.
| Generic { | ||
| ty: Box<AdapterType>, | ||
| args: Box<[AdapterType]>, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were writing this from scratch today, surely we wouldn't have a separate Generic, and would instead deeply nest all types properly? This seems like the right approach to properly integrate with other generics work in progress as well (such as generic enums - #4734, which had to deal with similar problems here in its own way).
I think we should try and generalize the approach a little better instead of stapling it on here.
So:
- If using a separate generic type, generalizing it to not be function-specific?
- Or, ideally, just making functions encode their deep tree to begin with?
|
@guybedford thanks for the review, well I submitted this a few months ago, and I believe there has been some changes and some generic features added since then as you mentioned, so I guess I first need to update myself with the new changes as I don't really remember the details of this PR 😄 so I'll try to free up some time and resolve the conflicts first and then address your review if that's ok with you, plz let me know |
Motivation
resolves #4451
Currently if a function/method has a generic return or arguments type like:
where
SomeTypeis just plain struct that impls wasm traits manually (not using wasm_bindgen macro on its definition, ie not js class) the generated typescript bindings wont correctly map the generic types and doesnt actually include them:Although user can use
unchecked_return_typeandunchecked_param_typeattribute to specify them, but this is not really ideal, it is fine and ok if there are a couple of functions to apply those attrs to, but if there are tons of functions and methods, it wont really end up good as they are unchecked and bug prone, however if the nested types can be processed regularly like the main type, this can simply be resolved, so on typescript we would get:Proposed Solution
Implement some logic to parse and map the types deeply, possibly recursively, until all nested types are parsed and accounted for in generated typescript.
There is a mapper currently that maps the rust return types to their equivalent js/ts types, but looks like it just checks 1 level deep, and doesnt go deeper than that, however with some tweaks, the same logic can be applied on nested types to correctly map the return type for the js/ts bindings.
We need to parse the args/return
syn::Types separately and map them with their nestedsyn::Types into their correspondingWasmDescribewhich later on can be encoded and then decoded intoDescriptorand next intoAdapterTypeso can be appended lastly to the parent TS type as generic if they happen to be generic.High-Level Implementation Details
We we would use native encoded/decoded
WasmDescribe::inform()for return and arguments types, meaning we will encode their wasm informs in thebackend::codegenforExportand then decode it atcli-support::descriptor::Functionwhich now has 2 new more fields:inner_ret_mapandargs_ty_map, these 2 field hold theDescriptorwhich now has 1 more variant calledDescriptor::Genericwhich exclusively added for this purpose and doesnt have any effect on any previous functionalities:that can hold the type map for both args and return type, now at
cli-support::wit::register_export_adapterwe convert thoseinner_ret_mapandargs_ty_mapfields intoAdapterTypeusing a newly added function calleddescribe_map_to_adapterwhere it just maps the givenDescriptorto itsAdapterTypeby also handling of nested generic types.Lastly when building the ts signature and js doc for the binding function/method, we will convert those generic
AdapterTypes into their corresponding js/ts types and append them to the main parent type.New tests are added under
typescript-testscrate testing complex nested generic types to be correctly mapped to their ts signature.This feature doesnt change any of the previous functionalities and logic, it simply is a new piece of logic that separately maps a
syn::Typewith any of its nestedsyn::Types to theirWasmDescribeand thenDescriptorand after that toAdapterTypecompletely separate to the how args and return type are resolved, and at the very last step of generating the ts signature, if the arg/return type is a generic, it will just append the generic to the actual parent type.Note
An attribute can be added to be able to turn this off for a given function/method, I have not added such attribute in this PR, but if that's desired, I can add it.