-
Notifications
You must be signed in to change notification settings - Fork 91
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 union type parameters #616
Conversation
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.
Looks good to me!
The only problem I see with this change is that the type in the method signature became harder to understand. signSelf(key_pair: KeyPair, method_query: UMethodQuery): void Developers must go to the method declaration to see what signSelf(key_pair: KeyPair, method_query: DIDUrl | string): void I'm not sure removing |
Turns out I forgot about the above since I was just looking at their guide: https://rustwasm.github.io/docs/wasm-bindgen/reference/attributes/on-rust-exports/typescript_type.html /// Duck-typed union to pass either a string or WasmDID as a parameter.
#[wasm_bindgen]
extern "C" {
#[wasm_bindgen(typescript_type = "DID | string")]
pub type UWasmDID;
}
/// Duck-typed union to pass either a string or WasmDIDUrl as a parameter.
#[wasm_bindgen]
extern "C" {
#[wasm_bindgen(typescript_type = "DIDUrl | string")]
pub type UWasmMethodQuery;
} With the revised code the Typescript function signature is as-desired: * @param {KeyPair} key_pair
* @param {DIDUrl | string} method_query
*/
signSelf(key_pair: KeyPair, method_query: DIDUrl | string): void; Thanks for the catch! |
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.
big improvement in my opinion
Description of change
This adds the
UMethodQuery
andUDID
(naming convention open to change) Typescript union types. This pattern allows us to create analogues for generic types in function parameters, such asMethodQuery
which takes either anIotaDIDUrl
orString
. This is to enhance the developer-friendliness of our Wasm API by removing unnecessarytoString
invocations and allowing pseudo-generic, polymorphic function parameters.export type UMethodQuery = DIDUrl | string;
export type UDID = DID | string;
Edit: this PR no longer exports the union types to Typescript, instead it injects the union into the function parameter definitions directly.
E.g.
With this change, all of the following work:
The alternative for
Client::resolve
andClient::resolveHistory
would be to enforce taking in aWasmDID
only, but error handling is more unwieldy than in Rust and users may extract DIDs as strings from JSON objects like credentials. The proposed pattern would still be useful forUMethodQuery
which matches the behaviour in Rust. Note this is only possible when all types in the union serialise to the same representation, since we useserde
to handle the conversion. Different union types are possible but would require an intermediary untagged enum to handle deserialisation with this method.Added
UDID
union type to take eitherDID
or astring
as a parameter.UMethodQuery
union type to take eitherDIDUrl
or astring
as a parameter.WasmNetwork::name
getter.Changed
Document::resolveMethod
parameter changed fromstring
toUMethodQuery
.Document::revokeMerkleKey
parameter changed fromstring
toUMethodQuery
.Document::signSelf
parameter changed fromstring
toUMethodQuery
.Document::diff
parameter changed fromstring
toUMethodQuery
.Client::resolve
parameter changed fromstring
toUDID
.Client::resolveHistory
parameter changed fromstring
toUDID
.WasmDID::network
is no longer a getter, just a regular function.WasmDID
JSON representation changed to matchIotaDID
.WasmDIDUrl
JSON representation changed to matchIotaDID
.Type of change
How the change has been tested
Wasm tests and examples pass locally.
Change checklist