Skip to content

web-sys: Bluetooth usability issues #2381

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

Open
chrysn opened this issue Dec 2, 2020 · 6 comments · Fixed by #2385 · May be fixed by #2387
Open

web-sys: Bluetooth usability issues #2381

chrysn opened this issue Dec 2, 2020 · 6 comments · Fixed by #2385 · May be fixed by #2387

Comments

@chrysn
Copy link
Contributor

chrysn commented Dec 2, 2020

Motivation

I'm porting Bluetooth using code over from Javascript, and am running into a few warts around how web-sys exposes them.

As I'm relatively new to web-sys usage, I don't have a clear plan forward with them, and they may or may not be related; in this issue I'd like to hash out how they are related, and what can reasonably be done within being a -sys crate.

Issues

  • navigator().bluetooth() may fail: Not all browsers have Bluetooth support, and Mozilla probably won't until it has vastly changed.

    Currently this causes a panic at with wasm-bindgen: imported JS function that was not marked as catch threw an error: getObject(...) is undefined; making .bluetooth() fallible may help here.

  • The request_device method is unusable as the default empty argument for options is not accepted, at least by Chrome. It either needs a concrete filter set, or acceptAllDevices set to true, in the options object.

  • Building the actual options object with a filter as a JsObject is comparatively verbose. To express

    navigator.bluetooth.requestDevice({
            filters: [{services: [serviceUuid]}]
            });

    I had to go through a serde object and wrote

    #[derive(serde::Serialize)]
    struct ServiceFilter {
        services: [&'static str; 1]
    }
    let services = [ServiceFilter { services: [UUID_US] }];
    b.request_device_with_options(
        web_sys::RequestDeviceOptions::new().filters(
            &wasm_bindgen::JsValue::from_serde(&services).unwrap()
    )).then([...]

    Making what MDN calls BluetoothScanFilters available in Rust might help, but so would an example of how to do this more in-line in the JsValue documentation. (I'm pretty sure there's a way to write RequestDeviceOptions::new().filters(js! [ {"services": [UUID_US]} ]), I just didn't find it). If that's accessible easily, it may even be worth considering to drop RequestDeviceOptions in favor of going all JavaScript object (given most of the possible options incur going to a JsValue right again), possibly through a TryInto (but that wouldn't be zero-cost; in a regular API one could probably go from accepting RequestDeviceOptions to accepting any Into which RequestDeviceOptions could then implement for compatibility, but a) this is all unstable anyway, and b) given there's codegen involved, such stunts may be hard).

@alexcrichton
Copy link
Contributor

Thanks for the report! I think it should be fine for us to change the WebIDL to accomodate this perhaps?

@chrysn
Copy link
Contributor Author

chrysn commented Dec 2, 2020

Reading further into WebIDL, that might easily make sense, eg. as

-  readonly attribute Bluetooth bluetooth;
+  readonly attribute Bluetooth? bluetooth;

and

-  Promise<BluetoothDevice> requestDevice(optional RequestDeviceOptions options = {});
+  Promise<BluetoothDevice> requestDevice(RequestDeviceOptions options = {});

. Is there any general guidance on which deviations from the upstream WebIDL are OK? (The Guide is silent on that.). Or would those be patches for upstream? On RequestDeviceOptions I'm a bit more clueless -- however, given it's a -sys, it may be up to other crates to create something builder-style that implements Into<RequestDeviceOptions> -- or where would something be done here?

@alexcrichton
Copy link
Contributor

There isn't necessarily a hard-and-fast guideline here but I think it's fine in this regard since it's unstable. There's not necessarily a canonical source-of-truth of WebIDL from what I've seen since browsers often have their own tweaks or perhaps a different snapshot than us.

In any case the unstable part of this is what matters and that gives us the freedom to change this at any time, so whatever works I think should be fine!

chrysn added a commit to chrysn-pull-requests/wasm-bindgen that referenced this issue Dec 2, 2020
* navigator.bluetooth is optional -- it may be manatory by the
  WebBluetooth spec, but the Rust code should still be able to handle
  the errors resulting from a browser not implementing the spec at all
  without panicking.

* requestDevice: While it's marked optional upstream, practically it's
  mandatory (the default value, an empty dictionary, is not accepted).

Contributes-To: rustwasm#2381
@chrysn
Copy link
Contributor Author

chrysn commented Dec 2, 2020

On the proposed patches I now have a PR open (#2385).

On the topic of how to get by the filters, I've found BluetoothLeScanFilterInit which helps over what I had to do with serde before -- but after the conversion, the information that filters is a sequence<BluetoothLEScanFilterInit> is lost, and only shows shows as JsValue. The two ways forward I see (but would need a great larger understanding of this system) are, in order of decreasing preference:

  • Express sequences in the type system. A sequence<BluetoothLEScanFilterInit> would become some type (possibly a generic Iterator<BluetoothLEScanFilterInit>, as that's what I'm using now with &[...].iter().collect::<js_sys::Array>()). That would hint the user at how to create them. It also makes the types stricter (you can't do .filter(some_arbitrary_jsvalue) any more -- that's kind of good but also very breaking.

  • For all cases where such information is lost, auto-generate documentation there; in this particular case it could be:

    The val argument should be a sequence of BluetoothLeScanFilterInit; even though that is not enforced at the Rust type level, it is expected on the JavaScript side. One way to create this argument is as &[val1, val2].iter().collect::<Array>().

  • If there is a way to get manually added comments from WebIDL to doc comments, equivalent (but possibly more precise) statements or examples could be added via there.

[edit: s/into_iter/iter/ due to rust-lang/rust#66145]

alexcrichton pushed a commit that referenced this issue Dec 2, 2020
* navigator.bluetooth is optional -- it may be manatory by the
  WebBluetooth spec, but the Rust code should still be able to handle
  the errors resulting from a browser not implementing the spec at all
  without panicking.

* requestDevice: While it's marked optional upstream, practically it's
  mandatory (the default value, an empty dictionary, is not accepted).

Contributes-To: #2381
@alexcrichton alexcrichton reopened this Dec 2, 2020
@alexcrichton
Copy link
Contributor

Ah yeah carrying over type parameters would be really nice to do and is an open issue with web-sys and its bindings. At the very least appending to the generated documentation would be great!

chrysn added a commit to chrysn-pull-requests/wasm-bindgen that referenced this issue Dec 3, 2020
... and report it during documentation

Closes: rustwasm#2381
@ChocolateLoverRaj
Copy link

I would prefer a Rust-friendly filters type, instead of having to to create JavaScript objects manually with Rust.

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