Added XMLHttpRequest support #50

keeslinp · 2017-12-25T08:08:07Z

At first, I just made a http_get function on the window that would make a get request and callback with the response, but I figured I might as well just give it a little more substance and create the whole object.

I think that it would be more cost-effective to couple these together so that there is less crossing of he wasm-js boundary and you'd only need to call one function. However, I saw somewhere that this library was trying to maintain low-level access so I figured this would be more flexible for a user. In comparison to the cost of a network request, the cost of crossing the boundary should be negligible.

koute · 2017-12-27T18:05:32Z

src/webapi/xml_http_request.rs

+    /// Send request on an open connection with the Value that is being sent
+    /// If using GET it would be Null
+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send)
+    pub fn send(&self, value: Value) {


Looking at the JS interface I think it would be better to provide multiple versions of this function instead of having it take a catch-all Value. Perhaps something like this?

fn send( &self ); fn send_with_string( &self, request_body: &str ); fn send_with_bytes( &self, request_body: &[u8] );

koute · 2017-12-27T18:08:41Z

src/webapi/xml_http_request.rs

+#[derive(Debug)]
+pub struct XMLHttpRequest {
+    reference: Reference,
+}


Please make this the same as other Reference wrappers. (e.g. look at src/webapi/file.rs for a minimal example) In this case the XMLHttpRequest derives from IEventTarget.

koute · 2017-12-27T18:11:10Z

src/webapi/xml_http_request.rs

+    /// First argument is the status code, the second argument is the response text
+    /// Makes use of onreadystatechange:
+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/onreadystatechange)
+    pub fn set_callback<F:FnMut(u32, String) + 'static>(&self, callback: F) {


Instead of ad hoc methods like this I'd prefer to do this through IEventTarget machinery we already have. (So instead of using this function the user would use add_event_listener from the IEventTarget interface.) In which case it would also be nice to have an usage example in the doc comments.

koute · 2017-12-27T18:12:48Z

src/webapi/xml_http_request.rs

+        }
+    }
+
+    /// Open connection with given method (ie GET or POST), and the url to hit


For every method you expose please take the docs from MDN and lightly edit them as needed, just as in your other PR. (:

koute · 2017-12-27T18:14:28Z

src/webapi/xml_http_request.rs

+            var callback = @{callback};
+            var xmlHttp = @{reference};
+            xmlHttp.onreadystatechange = function() {
+                if (xmlHttp.readyState == 4) {


The readyState should be exposed just like in (for example) file_reader.rs.

gabisurita · 2017-12-29T15:20:18Z

@keeslinp do you plan to address the changes pointed by @koute? I'm specially interested on this feature and would be happy to help if you're ok with that.

keeslinp · 2017-12-29T18:18:56Z

Yeah, I'm planning on doing that tonight or tomorrow. My schedule is a little weird at the moment cause of vacation. I've got some of it done but not all of it yet

…

On Fri, Dec 29, 2017, 8:21 AM Gabriela Surita ***@***.***> wrote: @keeslinp <https://github.com/keeslinp> do you plan to address the changes pointed by @koute <https://github.com/koute>? I'm specially interested on this feature and would be happy to help if you're ok with that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC6-bGnWgnkKjoS-CrjVs4fR7pxl9lrvks5tFQMzgaJpZM4RMLY6> .

CryZe · 2017-12-29T19:43:06Z

src/webapi/xml_http_request.rs

+    /// First argument is the status code, the second argument is the response text
+    /// Makes use of onreadystatechange:
+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/onreadystatechange)
+    pub fn set_callback<F:FnMut(u32, String) + 'static>(&self, callback: F) {


Should this be FnMut? I'm not entirely sure, but if the callback can only resolve once, it should probably be FnOnce

keeslinp · 2017-12-29T20:04:33Z

It can resolve more than once. The callback isn't directly connected with a sent request and so you could send more than once with the same callback. When I change it to use the event listener machinery I'll do it however it is done there.

…

On Fri, Dec 29, 2017, 12:43 PM Christopher Serr ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > +impl XMLHttpRequest { + /// Creates an empty XMLHttpRequest + pub fn new() -> XMLHttpRequest { + let reference: Reference = js! { + var xmlHttp = new XMLHttpRequest(); + return xmlHttp; + }.try_into().unwrap(); + XMLHttpRequest { + reference, + } + } + /// Sets the callback to call once a request completes + /// First argument is the status code, the second argument is the response text + /// Makes use of onreadystatechange: + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/onreadystatechange) + pub fn set_callback<F:FnMut(u32, String) + 'static>(&self, callback: F) { Should this be FnMut? I'm not entirely sure, but if the callback can only resolve once, it should probably be FnOnce — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC6-bAsh4-Sesn149ruOmeWv0fiszy47ks5tFUDMgaJpZM4RMLY6> .

keeslinp · 2017-12-30T10:30:09Z

Should be all done. It now much more reflects the javascript api. Honestly, I don't know if that's necessarily a good thing because it feels a little clunky but I can see how that's less surprising that having to learn a specific api for the callback.

koute

Looking better!

koute · 2017-12-31T10:31:34Z

src/webapi/event.rs

+/// called when the readystatechange event is fired,
+/// that is every time the readyState property of the XMLHttpRequest changes.
+///
+/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/onreadystatechange)


This should link to this and have the docs copied from this page:

https://developer.mozilla.org/en-US/docs/Web/Events/readystatechange

koute · 2017-12-31T10:32:41Z

src/webapi/xml_http_request.rs

+use webcore::try_from::TryInto;
+
+/// XMLHttpRequest object to make http requests
+/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest)


Please copy the first paragraph from MDN, and also put an extra newline before [(JavaScript docs)]

koute · 2017-12-31T10:33:08Z

src/webapi/xml_http_request.rs

+    convertible to EventTarget
+}
+
+/// A number indicating the state of the `XMLHttpRequest`.


In Rust it's not a number. (:

koute · 2017-12-31T10:34:09Z

src/webapi/xml_http_request.rs

+    Opened,
+    /// send() has been called, and headers and status are available.
+    HeadersReceived,
+    /// Downloading; responseText holds partial data.


Use Rust names here. (responseText -> response_text())

Also it would be nice to link to those methods you're referring to.

koute · 2017-12-31T10:34:59Z

src/webapi/xml_http_request.rs

+        match response {
+            Value::Null => None,
+            Value::String(resp) => Some(resp),
+            _ => None,


This should be an unreachable!.

koute · 2017-12-31T10:39:41Z

src/webapi/xml_http_request.rs

+        };
+    }
+
+    /// The abort() method aborts the request if it has already been sent.


You don't have to repeat the method name, e.g. The abort() method since it should be already obvious to the reader. (:

koute · 2017-12-31T10:40:27Z

src/webapi/xml_http_request.rs

+    }
+
+    /// The abort() method aborts the request if it has already been sent.
+    /// When a request is aborted, its readyState is changed to Done


readyState -> link to the ready_state method, status code -> link to the status method.

koute · 2017-12-31T10:42:28Z

src/webapi/xml_http_request.rs

+
+
+impl XMLHttpRequest {
+    /// Create new XMLHttpRequest


Minor grammar nitpick (this also applies to other doc comments): 'Create' -> 'Creates', and also a dot at the end and put identifiers (like XMLHttpRequest) inside backticks. (:

koute · 2017-12-31T10:43:27Z

src/webapi/xml_http_request.rs

+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send)
+    pub fn send(&self) {
+        js! {
+            @{self}.send(null);


No need to pass the null here. Just leave the parens empty.

koute · 2017-12-31T10:44:20Z

src/webapi/xml_http_request.rs

+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send)
+    pub fn send_with_bytes(&self, body: &[u8]) {
+        js! {
+            @{self}.send(@{body});


Use UnsafeTypedArray here to make it zero cost.

keeslinp · 2017-12-31T19:39:08Z

Haha docs are going to be the death of me :). It's a good learning experience for me though. I'll make these updates either tonight or tomorrow night depending how travel goes.

…

On Sun, Dec 31, 2017, 3:44 AM Koute ***@***.***> wrote: ***@***.**** requested changes on this pull request. Looking better! ------------------------------ In src/webapi/event.rs <#50 (comment)>: > @@ -1066,6 +1066,27 @@ reference_boilerplate! { convertible to ProgressRelatedEvent } +/// An EventHandler that is called whenever the readyState attribute changes. +/// The callback is called from the user interface thread. +/// The XMLHttpRequest.onreadystatechange property contains the event handler to be +/// called when the readystatechange event is fired, +/// that is every time the readyState property of the XMLHttpRequest changes. +/// +/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/onreadystatechange) This should link to this and have the docs copied from this page: https://developer.mozilla.org/en-US/docs/Web/Events/readystatechange ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > @@ -0,0 +1,129 @@ +use webapi::event_target::{IEventTarget, EventTarget}; +use webcore::value::{ + Reference, + Value, +}; +use webcore::try_from::TryInto; + +/// XMLHttpRequest object to make http requests +/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest) Please copy the first paragraph from MDN, and also put an extra newline before [(JavaScript docs)] ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + Reference, + Value, +}; +use webcore::try_from::TryInto; + +/// XMLHttpRequest object to make http requests +/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest ) +pub struct XMLHttpRequest( Reference ); + +reference_boilerplate! { + XMLHttpRequest, + instanceof XMLHttpRequest + convertible to EventTarget +} + +/// A number indicating the state of the `XMLHttpRequest`. In Rust it's not a number. (: ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + instanceof XMLHttpRequest + convertible to EventTarget +} + +/// A number indicating the state of the `XMLHttpRequest`. +/// +/// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/readyState) +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +pub enum ReadyState { + /// Client has been created. open() not called yet. + Unsent, + /// open() has been called. + Opened, + /// send() has been called, and headers and status are available. + HeadersReceived, + /// Downloading; responseText holds partial data. Use Rust names here. (responseText -> response_text()) Also it would be nice to link to those methods you're referring to. ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + 3 => Loading, + 4 => Done, + _ => unreachable!( "Unexpected value of XMLHttpRequest::readyState: {}", state ) + } + } + + /// Returns a DOMString that contains the response to the request as text, or None + /// if the request was unsuccessful or has not yet been sent. + /// + ///[(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseText) + pub fn response_text(&self) -> Option<String> { + let response = js!(return @{self}.responseText;); + match response { + Value::Null => None, + Value::String(resp) => Some(resp), + _ => None, This should be an unreachable!. ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + /// + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/readyState) + pub fn ready_state(&self) -> ReadyState { + use self::ReadyState::*; + let state: u16 = js!( return @{self}.readyState; ).try_into().unwrap(); + match state { + 0 => Unsent, + 1 => Opened, + 2 => HeadersReceived, + 3 => Loading, + 4 => Done, + _ => unreachable!( "Unexpected value of XMLHttpRequest::readyState: {}", state ) + } + } + + /// Returns a DOMString that contains the response to the request as text, or None DOMString -> string ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + } + + /// Returns a DOMString that contains the response to the request as text, or None + /// if the request was unsuccessful or has not yet been sent. + /// + ///[(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseText) + pub fn response_text(&self) -> Option<String> { + let response = js!(return @{self}.responseText;); + match response { + Value::Null => None, + Value::String(resp) => Some(resp), + _ => None, + } + } + + /// Returns an unsigned short with the status of the response of the request. More detailed docs would be nice. (First paragraph from MDN looks nice, without the remark that it's an unsigned short since we can already see that from the prototype.) ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + match response { + Value::Null => None, + Value::String(resp) => Some(resp), + _ => None, + } + } + + /// Returns an unsigned short with the status of the response of the request. + /// + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/status) + pub fn status(&self) -> u16 { + js!(return @{self}.status;).try_into().unwrap() + } + + /// Open connection with given method (ie GET or POST), and the url to hit + /// Async is set for now because it's more ergonimic in rust and it is good form in javascript "Async is set for now because it's more ergonimic in rust and it is good form in javascript" -> this seems unnecessary. You can document that it's asynchronous, but it's not necessary to explain it like this. ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + pub fn send_with_string(&self, body: String) { + js! { + @{self}.send(@{body}); + }; + } + + /// Send request on an open connection with a byte array body + /// + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send) + pub fn send_with_bytes(&self, body: &[u8]) { + js! { + @{self}.send(@{body}); + }; + } + + /// The abort() method aborts the request if it has already been sent. You don't have to repeat the method name, e.g. The abort() method since it should be already obvious to the reader. (: ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + js! { + @{self}.send(@{body}); + }; + } + + /// Send request on an open connection with a byte array body + /// + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send) + pub fn send_with_bytes(&self, body: &[u8]) { + js! { + @{self}.send(@{body}); + }; + } + + /// The abort() method aborts the request if it has already been sent. + /// When a request is aborted, its readyState is changed to Done readyState -> link to the ready_state method, status code -> link to the status method. ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + Unsent, + /// open() has been called. + Opened, + /// send() has been called, and headers and status are available. + HeadersReceived, + /// Downloading; responseText holds partial data. + Loading, + /// The operation is complete. + Done, +} + +impl IEventTarget for XMLHttpRequest {} + + +impl XMLHttpRequest { + /// Create new XMLHttpRequest Minor grammar nitpick (this also applies to other doc comments): 'Create' -> 'Creates', and also a dot at the end and put identifiers (like XMLHttpRequest) inside backticks. (: ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + /// Open connection with given method (ie GET or POST), and the url to hit + /// Async is set for now because it's more ergonimic in rust and it is good form in javascript + /// + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/open) + pub fn open(&self, method: &str, url: &str) { + js! { + @{self}.open(@{method}, @{url}, true); + }; + } + + /// Send request on an open connection with no data + /// + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send) + pub fn send(&self) { + js! { + @{self}.send(null); No need to pass the null here. Just leave the parens empty. ------------------------------ In src/webapi/xml_http_request.rs <#50 (comment)>: > + + /// Send request on an open connection with string body + /// + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send) + pub fn send_with_string(&self, body: String) { + js! { + @{self}.send(@{body}); + }; + } + + /// Send request on an open connection with a byte array body + /// + /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send) + pub fn send_with_bytes(&self, body: &[u8]) { + js! { + @{self}.send(@{body}); Use UnsafeTypedArray here to make it zero cost. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC6-bGI-widmAN-1HZ3RmbjaUeUohN92ks5tF2WegaJpZM4RMLY6> .

koute · 2017-12-31T19:40:05Z

Haha docs are going to be the death of me :)

Yeah, sorry for being so pedantic. (:

keeslinp · 2017-12-31T20:08:01Z

I'm sure many future users who are trying to find their way through the docs are going to thank you :)

…

On Sun, Dec 31, 2017, 12:40 PM Koute ***@***.***> wrote: Haha docs are going to be the death of me :) Yeah, sorry for being so pedantic. (: — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC6-bNJNHUR_TkwsynF_HhKpOnHqx4MTks5tF-MXgaJpZM4RMLY6> .

keeslinp · 2017-12-31T23:26:54Z

Should be fixed. Hopefully all the docs are in order now.

koute

Sorry for the delay!

Two more things I'd like changed.

There are also a few other nitpicks I have, but it's mostly fine so I'm not going to torment you with them. (:

Oh and, I can't physically merge it in as-is due to merge conflicts, so please rebase it on current master. (Basically just squash all of your commits into a single one with git rebase -i <first commit hash that isn't yours>, then do git fetch --all, then do git rebase master, and then you can git push --force to the branch.)

koute · 2018-01-02T17:48:49Z

src/webapi/xml_http_request.rs

+    /// Send request on an open connection with string body
+    ///
+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send)
+    pub fn send_with_string(&self, body: String) {


This should take &str; no real reason to take String.

koute · 2018-01-02T17:51:05Z

src/webapi/xml_http_request.rs

+impl XMLHttpRequest {
+    /// Creates new `XMLHttpRequest`.
+    pub fn new() -> XMLHttpRequest {
+        XMLHttpRequest(js!{


Actually, you should be able to convert it like this:

js!( return new XMLHttpRequest(); ).try_into().unwrap()

I think I tried that originally and I couldn't get it to work. I wonder if I set something up wrong with the boilerplate macro. I'll try it again tonight, maybe I just made a silly mistake.

I must have messed up something silly cause it worked fine this time. Thanks.

using reference pattern and event listener instead of custom callback fixed left out comma caused by rebase conflicts Updated docs for XMLHttpRequest Final changes for code review

keeslinp · 2018-01-03T03:21:41Z

Should be all better now :). Thanks for the help!

PS: Kudos on breaking a thousand stars, you've done a great job!

koute · 2018-01-03T17:01:16Z

PS: Kudos on breaking a thousand stars, you've done a great job!

Thanks! (:

Anyhow, this mostly looks fine now so I'm going to merge it in. Thank you for the contribution!

keeslinp mentioned this pull request Dec 25, 2017

Side effects (ie network requests) yewstack/yew#7

Closed

koute requested changes Dec 27, 2017

View reviewed changes

CryZe reviewed Dec 29, 2017

View reviewed changes

keeslinp force-pushed the xml-http-request branch from 8dd26a7 to ffd3652 Compare December 30, 2017 10:28

koute requested changes Dec 31, 2017

View reviewed changes

koute requested changes Jan 2, 2018

View reviewed changes

Added XMLHttpRequest support

cb11159

using reference pattern and event listener instead of custom callback fixed left out comma caused by rebase conflicts Updated docs for XMLHttpRequest Final changes for code review

keeslinp force-pushed the xml-http-request branch from 2f83742 to cb11159 Compare January 3, 2018 03:21

koute merged commit 7bf0d86 into koute:master Jan 3, 2018

gabisurita mentioned this pull request Jan 3, 2018

Implement set and get headers for XMLHttpRequest #66

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added XMLHttpRequest support #50

Added XMLHttpRequest support #50

keeslinp commented Dec 25, 2017

koute Dec 27, 2017

koute Dec 27, 2017

koute Dec 27, 2017

koute Dec 27, 2017

koute Dec 27, 2017

gabisurita commented Dec 29, 2017

keeslinp commented Dec 29, 2017 via email

CryZe Dec 29, 2017

keeslinp commented Dec 29, 2017 via email

keeslinp commented Dec 30, 2017

koute left a comment

koute Dec 31, 2017

koute Dec 31, 2017

koute Dec 31, 2017

koute Dec 31, 2017

koute Dec 31, 2017

koute Dec 31, 2017

koute Dec 31, 2017

koute Dec 31, 2017

koute Dec 31, 2017

koute Dec 31, 2017

keeslinp commented Dec 31, 2017 via email

koute commented Dec 31, 2017

keeslinp commented Dec 31, 2017 via email

keeslinp commented Dec 31, 2017

koute left a comment •

edited

koute Jan 2, 2018

koute Jan 2, 2018

keeslinp Jan 2, 2018

keeslinp Jan 3, 2018

keeslinp commented Jan 3, 2018 •

edited

koute commented Jan 3, 2018

Added XMLHttpRequest support #50

Added XMLHttpRequest support #50

Conversation

keeslinp commented Dec 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabisurita commented Dec 29, 2017

keeslinp commented Dec 29, 2017 via email

Choose a reason for hiding this comment

keeslinp commented Dec 29, 2017 via email

keeslinp commented Dec 30, 2017

koute left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keeslinp commented Dec 31, 2017 via email

koute commented Dec 31, 2017

keeslinp commented Dec 31, 2017 via email

keeslinp commented Dec 31, 2017

koute left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keeslinp commented Jan 3, 2018 • edited

koute commented Jan 3, 2018

koute left a comment •

edited

keeslinp commented Jan 3, 2018 •

edited