-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement getters for WebIDL dictionaries #3933
Conversation
#unstable_attr | ||
#getter_trait_doc_comment | ||
pub trait #getter_trait_name { | ||
#(#getter_trait_fields)* | ||
} | ||
|
||
#unstable_attr | ||
impl #getter_trait_name for #name { | ||
#(#getter_trait_impl_fields)* | ||
} |
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.
That's the main addition of this PR...
#unstable_attr | ||
#cfg_features | ||
#doc_comment | ||
#unstable_docs | ||
fn #name(&self) -> #return_ty; |
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.
...with getter signatures that are defined here...
fn #name(&self) -> #return_ty { | ||
self.#shim_name() | ||
} |
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.
...and implemented here
I agree. We should bite the bullet, break the compatibility and do it (in another PR ofc). |
I like the idea of having traits here. We could probably go even further - have a trait with a WebIDL-generated declaration of the whole interface of a given type - not just for getters. That way we can include getters, setters and methods (both static and not). I think we could create an issue to discuss this further, if this sounds like a viable direction to explore. |
I think you are referring to what I have an mind, but just to elaborate the thought: I'm not fully sure yet how hard it would be to make it possible, but if that would allow e.g. implementing |
crates/webidl-tests/dictionary.rs
Outdated
let mut many_types = ManyTypes::new(); | ||
many_types.a("a"); | ||
many_types.n1(1); | ||
many_types.n2(2); | ||
many_types.n3(3); | ||
many_types.n4(4); | ||
many_types.n5(5); | ||
many_types.n6(6); | ||
let mut other_dict = OtherDict::new(); | ||
other_dict.a(42); | ||
many_types.c(&other_dict); | ||
|
||
{ | ||
use crate::generated::ManyTypesGetters; | ||
assert_eq!(many_types.a(), "a"); | ||
assert_eq!(many_types.n1(), 1); | ||
assert_eq!(many_types.n2(), 2); | ||
assert_eq!(many_types.n3(), 3); | ||
assert_eq!(many_types.n4(), 4); | ||
assert_eq!(many_types.n5(), 5); | ||
assert_eq!(many_types.n6(), 6); | ||
assert_eq!(many_types.c(), other_dict); | ||
} |
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.
Unfortunately, due to the signature name clash this is a bit awkward to use right now.
If you move the use crate::generated::ManyTypesGetters;
to the parent block, this test breaks.
See #3934 Let's take a If it was a trait, could just implement it for a Then, if needed, we could very elegantly extend With the given WebIDL: WebIDL
[Exposed=*, Transferable]
interface WritableStream {
constructor(optional object underlyingSink, optional QueuingStrategy strategy = {});
readonly attribute boolean locked;
Promise<undefined> abort(optional any reason);
Promise<undefined> close();
WritableStreamDefaultWriter getWriter();
}; [Exposed=(Window,Worker), SecureContext, Transferable]
interface WebTransportSendStream : WritableStream {
attribute WebTransportSendGroup? sendGroup;
attribute long long sendOrder;
Promise<WebTransportSendStreamStats> getStats();
WebTransportWriter getWriter();
}; The traits would look somewhat like this: Traits
trait WritableStream: Sized + JsAny {
fn new() -> Self;
fn new_with_underlying_sink(underlying_sink: &impl JsObject) -> Self;
// ...
fn locked(&self) -> bool;
fn abort(&self) -> Promise<()>;
fn abort_with_reason(&self, reason: &impl JsAny) -> Promise<()>;
fn close(&self) -> Promise<()>;
fn get_writer(&self) -> Result<impl WritableStreamDefaultWriter>;
}
trait WebTransportSendStream: WritableStream + JsAny {
fn send_group(&self) -> Option<impl WebTransportSendGroup>;
fn set_send_group(&self, value: &impl WebTransportSendGroup) -> Option<impl WebTransportSendGroup>;
fn unset_send_group(&self);
fn send_order(&self) -> i64;
fn set_send_order(&self, value: i64);
fn unset_send_order(&self);
fn get_stats(&self) -> Promise<impl WebTransportSendStreamStats>;
fn get_writer(&self) -> Result<impl WebTransportWriter>;
} and impls like this: Impls
impl WritableStream for JsValue {
fn new() -> Self {
__shim_WritableStream_new()
}
fn new_with_underlying_sink(underlying_sink: &impl JsObject) -> Self {
__shim_WritableStream_new_with_underlying_sink(underlying_sink)
}
// ... and so on.
}
impl WebTransportSendStream for JsValue {
fn send_group(&self) -> Option<impl WebTransportSendGroup> {
__shim_WebTransportSendStream_send_group()
}
// ... and so on.
} A couple things to note:
|
Cool, tests pass now. Just a note that this will break for people that use |
I think that the easiest way forward would be to just implement getters as Obviously this isn't Rusty, but I believe its better then introducing a trait for each type while still remaining backwards compatible. @Liamolucko would appreciate your input as well. |
I agree; I think the messiness of adding the extra traits outweighs the benefit of the better names. I'm also not really comfortable with the potential breakage of |
Do you keep track of changes that you'd want to include in a future breaking change? Since arguably this would be a great candidate, and the previous naming does not follow common Rust guidelines. I can't imagine anyone complaining that their setters are now called Even though I disagree with the naming, happy to make the necessary changes to |
We don't, mainly because we don't see a breaking change on the horizon anytime soon.
Indeed that's how we would like to proceed here. |
I was thinking about this, and there is a breaking change that I'd want to merge in with the async fs - that one didn't get any attention for a while not... So, maybe it is time to fork? It would not be too bad to have an experimental fork, especially since most of the code is generated. I'd very much welcome the changes in here, and my use cases really really don't care about the breaking changes - as long as the underlying objects and be converted back and forth between the I can spend some resources setting up the repos and etc, but it would be great to collab with someone and formulate the needs for the fork upfront - like having more frequent breaking releases to allow advancement. |
Replaced with #3993. |
Heyo,
this PR implements getters for WebIDL dictionaries. The diff is bit noisy due to the autogenerated files; you can look at the actual changes to the code generation in the last two files
crates/webidl/src/generator.rs
andcrates/webidl/src/lib.rs
and corresponding tests incrates/webidl-tests/dictionary.rs
.Currently, setters are implemented as
my_property(&mut self, val: ...)
. To avoid breaking backwards compatibility, I have generated a new traitMyDictionaryTypeGetters
for everyMyDictionaryType
. It contains definitions in the form ofmy_property(&self) -> ...
.Personally, I would suggest to rename all setters to
set_my_property
and place themy_property
getters back in theimpl MyDictionaryType
as conventionally used in Rust. Preferably in another PR since as already mentioned would break backwards compatibility.For the dictionary getter return types I have applied the same logic that is currently used for Interface getters:
wasm-bindgen/crates/webidl/src/lib.rs
Lines 688 to 692 in e78db23
wasm-bindgen/crates/webidl/src/generator.rs
Lines 298 to 300 in e78db23
This should close #1793 and #2921, and address point 1 of #3921.