[WIP] Add mid-level bindings for XmlHttpRequest

[tomhoule]

This is intended to implement the feature proposed in #37

TODO before this can be merged:

• Documentation.
• Realistic examples.
• Avoid leaking the event listeners (maybe use gloo-events?)
• More tests.
• More thoughtful error handling.

It would also be nice to figure out a way to spin up a test http server in the background for browser tests, but I haven't found a straightforward way to do it without running a separate command so far.

[tomhoule]

should we assume this is safe because all browsers that support webassembly implement XmlHttpRequest, or check whether the constructor is defined?

[Pauan]

If an API predates WebAssembly, then yes we can safely assume it exists.

In addition, XMLHttpRequest goes all the way back to IE7! So we can safely assume it exists regardless.

Personally, I would just use unwrap_throw, but I don't have strong feelings about it.

[fitzgen]

Avoid leaking the event listeners (maybe use gloo-events?)

Yes, let's definitely build on top of gloo_events instead of using onfoo properties. This way, you can have more than one listener for each kind of event, and we get to exercise our own APIs a bit to make sure they're working smoothly.

[tomhoule]

Just an update to say this PR isn't abandoned, I hope to find time to work on documenting and testing more soon. I am also waiting on gloo-events (I tried using it but gave up on panics in tests) to finish shaping the API.

[tomhoule]

I think this is now ready for a bit more feedback. Here are a few doubts/questions I have regarding this implementation:

• is impl AsRef<web_sys::EventTarget> for XmlHttpRequest (meant for integration with gloo_events) a good idea? Would a regular method be better?
• The doctests crash because they can't run in non-wasm environments, should they be completely disabled?
• send_no_body: we could alternatively have send() and send_with_body(body), or have send take an Option<impl XhrBody>. The second solution seems error-prone to me, but it's the only way to avoid having two methods. Is there a convention there?
• The test suite is very limited because I haven't figured out a way to spawn a real HTTP server in the background to test a lot more thoroughly. At least not without having a test script wrapping wasm-pack test.

[tomhoule]

TODO: we can test abort events easily.

[tomhoule]

It looks like the event handlers question is being discussed on #51, I'll read the conversation soon to adapt this.

[fitzgen]

is impl AsRef<web_sys::EventTarget> for XmlHttpRequest (meant for integration with gloo_events) a good idea? Would a regular method be better?

Seems good to me. Unlike somethign like Deref or IntoIterator, there can be more than one AsRef impl for a type (eg AsRef<T> and AsRef<U>) so it doesn't cut off other potential additions in the future.

The doctests crash because they can't run in non-wasm environments, should they be completely disabled?

In these cases, I like to use ```no_run blocks, which still verify that the example compiles, but does not attempt to run/test them.

send_no_body: we could alternatively have send() and send_with_body(body), or have send take an Option<impl XhrBody>. The second solution seems error-prone to me, but it's the only way to avoid having two methods. Is there a convention there?

I don't have strong opinions, but this item in the API guidelines would suggest either two different methods or one method with a custom enum:

enum SendBody<B: XhrBody> {
    None,
    Body(B),
}

impl XmlHttpRequest {
    pub fn send<B: XhrBody>(&self, body: SendBody<B>) { ... }
}

We could also have the union of these methods:

impl XmlHttpRequest {
    pub fn send<B: XhrBody>(&self, body: SendBody<B>) { ... }
    pub fn send_with_body<B: XhrBody>(&self, body: B) { ... }
    pub fn send_without_body(&self) { ... }
}

Again, I don't really have strong opinions either way.

The test suite is very limited because I haven't figured out a way to spawn a real HTTP server in the background to test a lot more thoroughly. At least not without having a test script wrapping wasm-pack test.

Another option is to load some JS snippet to monkey patch the API before the wasm and its JS glue imports it, and then mock up the behavior as needed:

// gloo/crates/xhr/tests/mock.js

window.XMLHttpRequest.prototype.open = function(...) { ... };
window.XMLHttpRequest.prototype.send = function(...) { ... };

export function setNextRequestStatus(code) {
    // stash some state so that the next XHR we send will return the given HTTP status code..
}

// gloo/crates/xhr/tests/web.rs

#[wasm_bindgen(module = "/tests/mock.js")]
extern "C" {
    #[wasm_bindgen(js_name = setNextRequestStatus)]
    fn set_next_request_status(code: u32);
}

[tomhoule]

Thanks for all the advice - there are improvements to make to this PR, I'll try to find the time to work on it asap, it's a bit complicated now as I am moving :)

[yoshuawuyts]

Would it make sense for this to be an http::HeaderMap instead?

[tomhoule]

yep, this needs to be case insensitive, I realized writing tests for it

[yoshuawuyts]

I would've expected all the send_* methods to return Result instead of panicking. Could you expand on the reasoning here why panicking is preferred over Result?

[tomhoule]

I went the shortest route for error handling because there was no clear prior art/guideline there, but I agree that in principle a Result with a type error variant is ideal.

[yoshuawuyts]

I'm noticing there's quite a large API surface here, and I feel I have quite a bit of feedback I might want to share. Similar to the File API (#76), would it perhaps make sense to create consensus on the API in an RFC first, before moving to an implementation PR?

---

@tomhoule I understand proposing this might feel a bit frustrating, because you've put a lot of work in. But I think if we do this well we might be able to land the implementation faster because we're splitting the design stage from the implementation allowing us to focus on one set of challenges at the time.

Also it might create a valuable resource to refer to when we inevitably want to tackle the Fetch / AbortController APIs. Hope it's okay that I'm bringing this up!

[tomhoule]

sure - there's a lot to figure out, especially around error handling. I took up the PR thinking of getting this merged first and then polishing up, but we can discuss first (as you may have noticed I am very short on open source time at the moment though, sorry in advance if I can't get very involved). Let's close this. Should the discussion continue on #37?