Skip to content
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

Include Record Type #3587

Closed
OptimisticPeach opened this issue Aug 31, 2023 · 2 comments · Fixed by #4030
Closed

Include Record Type #3587

OptimisticPeach opened this issue Aug 31, 2023 · 2 comments · Fixed by #4030

Comments

@OptimisticPeach
Copy link

Motivation

In the GpuDeviceDescriptor generated struct in web_sys, we don't get access to the required_limits api since it has a WebIDL type of record<DOMString, GPUSize64> requiredLimits;. The relevant piece of code which omits the record type is here.

WGPU needs access to this particular field to be able to request a device with limits other than the bare minimum.

Proposed Solution

A kind of Record type added into js_sys, since it seems that the blocker (reasonably) is the lack of a Record type in js_sys. Exactly how this would look is something I'm not sure, hence why this is an issue and not a PR, and am welcoming discussion.

Alternatives

We could also simply ignore the type constraints we are supposed to follow for record types and allow any keys. However, this would be unwanted since you could get silent or cryptic errors since the api you are communicating with may recieve unexpected types as the keys.

@Liamolucko
Copy link
Collaborator

It seems like records are only ever used as inputs in WebIDL, so it could also make sense to have a builder-style API for them similar to what's done for dictionaries. Something like:

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(extends = Object)]
    pub type GpuDeviceDescriptorRequiredLimits;

    #[wasm_bindgen(method, indexing_setter)]
    fn add_entry(this: &GpuDeviceDescriptorRequiredLimits, key: &str, value: f64);
}

impl GpuDeviceDescriptorRequiredLimits {
    pub fn new() -> Self {
        Object::new().unchecked_into()
    }

    pub fn entry(&mut self, key: &str, value: f64) -> &mut Self {
        // alternatively this could use `Reflect::set`
        self.add_entry(key, value);
        self
    }
}

@daxpedda
Copy link
Collaborator

daxpedda commented Sep 2, 2023

It seems like records are only ever used as inputs in WebIDL, so it could also make sense to have a builder-style API for them similar to what's done for dictionaries.

Considering how using a builder-style API for dictionaries didn't turn out so well, I would prefer not to repeat this mistake. Especially the whole &mut self and using Reflect. I think we should just supply getters and setters as with any other normal type.


Happy to review a PR adding this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants