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

Added WebUSB API #2345

Merged
merged 1 commit into from
Nov 11, 2020
Merged

Added WebUSB API #2345

merged 1 commit into from
Nov 11, 2020

Conversation

hajifkd
Copy link
Contributor

@hajifkd hajifkd commented Nov 6, 2020

I added WebUSB IDL file. I took the spec from here.

In addition to adding the WebUSB spec, I added DataView as one of the builtin types.
I guess usually generated features in crates/web-sys/feature are to be copied to crates/web-sys/Cargo.toml. Since DataView is actually a builtin type but not dealt so, DataView feature is generated in Rust codes but not in crates/web-sys/feature. Thus, one need to add it to Cargo.toml by hand. This causes future confusion and to be fixed, I think. (IMHO, this should have been done in #2316.)
One drawback is that existing projects using WebBluetooth API via wasm-bindgen are broken, since DataView feature must be used there but it is now removed. But since WebBluetooth API is unstable feature of web-sys, it should be acceptable.

@alexcrichton
Copy link
Contributor

Looks good to me, thanks for this! Also agreed on the action taken with DataView, and we can always tweak it if necessary and breakage comes up.

@hajifkd
Copy link
Contributor Author

hajifkd commented Nov 9, 2020

I noticed that FrozenArray is not implemented in webidl crate and some API does not work. The same problem actually exists for other APIs, like Clipboard and Bluetooth.
In my understanding, FrozenArray is a immutable array in Javascript. Since the mutability is not completely reflected in web-sys / js-sys crates (as long as I guess - is it correct?), I feel just generating the same codes as Sequence sounds fine. (I guess unifying Sequence and FrozenArray in

IdlType::FrozenArray(_idl_type) => Err(TypeError::CannotConvert),
// webidl sequences must always be returned as javascript `Array`s. They may accept
// anything implementing the @@iterable interface.
IdlType::Sequence(_idl_type) => match pos {
TypePosition::Argument => Ok(js_value),
TypePosition::Return => Ok(js_sys("Array")),
},
would work.)

Or, it might be also fine to intepret any type for which to_syn_type returns Err as JsValue in Rust although it partially spoils the advantage of the typed language.

What do you think?

@alexcrichton
Copy link
Contributor

Ah yeah makes sense! I think that's the JS type appearing there anyway so seems reasonable to just do the same thing as Sequence. Thanks for catching this!

@hajifkd
Copy link
Contributor Author

hajifkd commented Nov 10, 2020

OK, will do!
Also I am going to add tests for DataView and FrozenArray in webidl-tests.
I cannot find any test for Sequence and Promise so I will add them as well.

@hajifkd
Copy link
Contributor Author

hajifkd commented Nov 10, 2020

Is there any reason to ignore generics arguments of Sequence? It seems to be possible to append it. The compatibility for these APIs would be broken, though. In addition, Promise can be also typed.

@alexcrichton
Copy link
Contributor

Currently we don't have a great way to thread it through to the Rust definition right now and doing so would probably be a pretty large breaking change for web-sys as well. That being said it'd be better if we could do it so I'd love to have it on the docket for a future release. Was there anything else you wanted to do here before landing though?

@hajifkd
Copy link
Contributor Author

hajifkd commented Nov 10, 2020

That makes sense. Let me squash all commits before merging.

@alexcrichton alexcrichton merged commit b87a901 into rustwasm:master Nov 11, 2020
@alexcrichton
Copy link
Contributor

Thanks again!

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

Successfully merging this pull request may close these issues.

2 participants