547091 - implement request module for making network requests

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

We should implement the request module for making network requests that is proposed in JEP 109.

Comment 2

[Atul Varma [:atul]]

btw, it would probably be useful for you to just build on the 'xhr' module in jetpack-core for this, since that module takes care of a lot of the "best practices" such as keeping track of all open XHRs and aborting them on unload, ensuring client exceptions are logged by Jetpack and don't propagate into Gecko, etc.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (WIP)

just throwing my WIP up.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

AIUI, this bug will need the mock xhr (bug 562234) in order to successfully test

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2 (WIP)

Still a WIP, but I think the implementation is all working. I'm going to write a little PHP script to throw on my server to start properly testing (not just look at console.log output). When we get mock xhr, I'll convert over.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3 (WIP)

Still a WIP, but I wanted to add the tests that I had so far to show some coverage.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 447657 [details] [diff] [review]
Patch v0.3 (WIP)

Looking good!

+  this.get = function () {
+    if (!request)
+      makeRequest("GET");
+  };
+
+  this.post = function () {
+    if (!request)
+      makeRequest("POST");
+  };

These should return the request object, so it's possible to do:

  let request = Request({ ... }).get();

We should also consider making it possible to configure a request object from these methods, so one can do:

  let request = Request();
  // then later...
  request.get({ ... });
  // then later, without creating a new instance...
  request.get({ ... });


+  // At this point, if it's not an object, we can't do anything with it, so
+  // return null. We should be ok thanks to validateOptions, but just to be
+  // sure...
+  if (typeof(content) != "object" || !content ||
+      Object.prototype.toString.call() == "[object Array]")
+    return null;

Hmm, .call(content)?


+  // this.json should be the JS object, so we need to attempt to parse it.
+  //XXXzpao do we throw? return null? undefined?

Return null (just like .xml does when the response content is not valid XML).

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #7)
> (From update of attachment 447657 [details] [diff] [review])
> Looking good!
> 
> +  this.get = function () {
> +    if (!request)
> +      makeRequest("GET");
> +  };
> +
> +  this.post = function () {
> +    if (!request)
> +      makeRequest("POST");
> +  };
> 
> These should return the request object, so it's possible to do:
> 
>   let request = Request({ ... }).get();

That's easy enough. I assume you mean our object and not the actual xhr object.

> We should also consider making it possible to configure a request object from
> these methods, so one can do:
> 
>   let request = Request();
>   // then later...
>   request.get({ ... });
>   // then later, without creating a new instance...
>   request.get({ ... });

I disagree here. I think we should do one or the other - either you create a Request object or you require("request").get|post. I think (new Request()).get({...}) is bad syntax. It sort of implies that a request can be reused, when in fact it can't.

I wouldn't mind adding .get|post to the exported object - it would just be shorthand for what you wrote.

> +  // At this point, if it's not an object, we can't do anything with it, so
> +  // return null. We should be ok thanks to validateOptions, but just to be
> +  // sure...
> +  if (typeof(content) != "object" || !content ||
> +      Object.prototype.toString.call() == "[object Array]")
> +    return null;
> 
> Hmm, .call(content)?

True. I really wanted to use Drew's getTypeOf from api-utils since it's a more complete type checker and I'm really just duplicating a large part of it here. Perhaps we can export that?

> +  // this.json should be the JS object, so we need to attempt to parse it.
> +  //XXXzpao do we throw? return null? undefined?
> 
> Return null (just like .xml does when the response content is not valid XML).

Sounds good.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #8)
> > These should return the request object, so it's possible to do:
> > 
> >   let request = Request({ ... }).get();
> 
> That's easy enough. I assume you mean our object and not the actual xhr object.

Right!


> I disagree here. I think we should do one or the other - either you create a
> Request object or you require("request").get|post. I think (new
> Request()).get({...}) is bad syntax. It sort of implies that a request can be
> reused, when in fact it can't.

Hmm, I was under the impression that requests could be reused, which was the whole point of get({...}).  If that's not that case, then you're absolutely right that get({...}) doesn't make any sense.


> True. I really wanted to use Drew's getTypeOf from api-utils since it's a more
> complete type checker and I'm really just duplicating a large part of it here.
> Perhaps we can export that?

Sure, makes sense!

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.4

Ok, so this is code only (no docs yet). I realized it still needs a test for testing reusablity (or lack thereof) of a Request.

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #9)
> Hmm, I was under the impression that requests could be reused, which was the
> whole point of get({...}).  If that's not that case, then you're absolutely
> right that get({...}) doesn't make any sense.

Per our discussion offline, a Request won't be reusable for now. We'll reevaluate later.
 
> > True. I really wanted to use Drew's getTypeOf from api-utils since it's a more
> > complete type checker and I'm really just duplicating a large part of it here.
> > Perhaps we can export that?
> 
> Sure, makes sense!

Turns out I didn't really need to do that. It might come in handy later, but not yet.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 450767 [details] [diff] [review]
Patch v0.4

>diff --git a/packages/jetpack-core/lib/request.js b/packages/jetpack-core/lib/request.js

>+  // Explicitly return null if we have null, and empty string, or empty object.

Nit: and -> an


>+  if (typeof(content) == "string") {
>+    //XXXzpao urlencode this?
>+    return content;

No, unfortunately we can't tell if this represents a single value that is intended to be the entire query string or a sequence of key/value pairs (in which case the consumer must already have encoded it).

So we have to make the consumer do it.  But we should definitely encode the keys and values of an object argument (as you are already doing).  And it might be worth providing an |encode| method of the Request object (or just the exports object) that exposes fixedEncodeURIComponent at some point, although that doesn't need to be in this patch.


>+  // At this point we have a k:v object. Iterate over it and encode each value.
>+  // Arrays and nested objects will get encoded as needed. For example...
>+  //
>+  //   { foo: [1, 2, { omg: "bbq", "all your base!": "are belong to us" }], bar: "baz" }
>+  //
>+  // will be encoded as
>+  //
>+  //   foo[0]=1&foo[1]=2&foo[2][omg]=bbq&foo[2][all+your+base!]=are+belong+to+us&bar=baz
>+  //
>+  // Keys (including "[" and "]") and values will be encoded with
>+  // fixedEncodeURIComponent before returning.
>+  //
>+  // Execution was inspired by jQuery, but some details have changed and numeric
>+  // array keys are included (whereas they are not in jQuery).

Excellent!


>+  // this.headers also should be a JS object, so we need to split up the raw
>+  // headers string provided by the request.
>+  xpcom.utils.defineLazyGetter(this, "headers", function () {
>+    let _headers = {};
>+    // Since getAllResponseHeaders() will return null if there are no headers,
>+    // defend against it by defaulting to ""
>+    let rawHeaders = request.getAllResponseHeaders() || "";
>+    rawHeaders.split("\u000a").forEach(function (h) {
>+      let [key, val] = h.split(":");

Based on my reading of the spec, it appears that HTTP allows unencoded colons in header values, so this should do h.split(":", 1) to prevent splitting the value into multiple result array items.


>+      // We end up with an extra item after splitting rawHeaders, so splitting
>+      // that line gives us an empty key & an undefined val, which we don't want
>+      // to add to our headers.
>+      if (val !== undefined) {
>+        // There are empty spaces after splitting, so we need to get rid of those.
>+        _headers[key.trim()] = val.trim();

HTTP (section 4.2 Message Headers) allows multiple headers with the same key, specifying that they are equivalent to a single header with multiple comma-separated values.

It's not clear if XHR already so concatenates multiple headers with the same key; if not, this should do so, since we can't represent such headers separately in a JS object.  But that doesn't have to be fixed in this patch, given that it is probably pretty rare (even if not handled by XHR).  Instead, you can just put a TODO here.

HTTP also allows headers to span multiple lines (whose intervening whitespace can be collapsed to a single space).  That too can be TODOed (if not already handled by XHR).


>+// apiUtils.validateOptions doesn't give the ability to easily validate single
>+// options, so this is a wrapper that provides that ability.
>+function OptionsValidator(rules) {

Yum!  Can we get this added to api-utils?


>diff --git a/packages/jetpack-core/tests/test-request.js b/packages/jetpack-core/tests/test-request.js

>+    url: "http://playground.zpao.com/jetpack/request/nonexistant.php",

Nit: existant -> existent


Otherwise, this looks great.  It just needs docs, and then it'll be good to go!

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #12)
> And it might
> be worth providing an |encode| method of the Request object (or just the
> exports object) that exposes fixedEncodeURIComponent at some point, although
> that doesn't need to be in this patch.

That method isn't something I would have done had I not looked at MDC. It seems that with the exception of s/%20/+/ it seems other frameworks just use encodeURIComponent. If/When we export it, it might make more sense elsewhere. makeQueryString might also be something good to export, though perhaps from XHR, for consumers of the low level API that might be doing something more complex.

> >+    let rawHeaders = request.getAllResponseHeaders() || "";
> >+    rawHeaders.split("\u000a").forEach(function (h) {
> >+      let [key, val] = h.split(":");
> 
> Based on my reading of the spec, it appears that HTTP allows unencoded colons
> in header values, so this should do h.split(":", 1) to prevent splitting the
> value into multiple result array items.

Good call. I hadn't actually looked at the spec, though I am now.

> >+      // We end up with an extra item after splitting rawHeaders, so splitting
> >+      // that line gives us an empty key & an undefined val, which we don't want
> >+      // to add to our headers.
> >+      if (val !== undefined) {
> >+        // There are empty spaces after splitting, so we need to get rid of those.
> >+        _headers[key.trim()] = val.trim();
> 
> HTTP (section 4.2 Message Headers) allows multiple headers with the same key,
> specifying that they are equivalent to a single header with multiple
> comma-separated values.
> 
> It's not clear if XHR already so concatenates multiple headers with the same
> key; if not, this should do so, since we can't represent such headers
> separately in a JS object.  But that doesn't have to be fixed in this patch,
> given that it is probably pretty rare (even if not handled by XHR).  Instead,
> you can just put a TODO here.

Is this something that if encountered, we might want to stick the values into an array?

> HTTP also allows headers to span multiple lines (whose intervening whitespace
> can be collapsed to a single space).  That too can be TODOed (if not already
> handled by XHR).

I'll play around with these and see and see if there are some easy fixes (if needed). If so, I'll add them. If not, I'll TODO them.

> >+// apiUtils.validateOptions doesn't give the ability to easily validate single
> >+// options, so this is a wrapper that provides that ability.
> >+function OptionsValidator(rules) {
> 
> Yum!  Can we get this added to api-utils?

That's going to be part of bug 569122.

Comment 14

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Heh, so I somehow missed this before... according to the JEP, onComplete is supposed to get a response argument. I never did that. I'll of course fix that and also update the tests so they follow the correct usage.

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.5

(In reply to comment #14)
> Heh, so I somehow missed this before... according to the JEP, onComplete is
> supposed to get a response argument. I never did that. I'll of course fix that
> and also update the tests so they follow the correct usage.

I lied. Talked to Myk and since the response is available as this.response, we'll just skip modifying onComplete.

Otherwise, it has docs now (I hate writing docs) and the only other major change was rewriting the response header processing since it was wrong in several ways.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 451427 [details] [diff] [review]
Patch v0.5

>diff --git a/packages/jetpack-core/docs/request.md b/packages/jetpack-core/docs/request.md

>+The `request` module provides a wrapper around the low level
>+[xhr](#module/jetpack-core/xhr) module, making it easier to use for many cases.

Nit: I think the actual value here is providing a simple, powerful way to make network requests rather than being a wrapper, so the intro would be better as something like:

  The `request` module lets you make simple, powerful network requests.

You might then also say:

  It wraps the [xhr](#module/jetpack-core/xhr) module.

But I don't think it's necessary, and perhaps it isn't even desirable, since we want most developers to be using this one.


>+Each `Request` object is designed to be used once. Once `get` or `post` are
>+called, attempting to call either will throw an error. Since the request is not
>+being made by any particular website, requests made here are not subject to the
>+same-domain restriction that requests made in content are subject to.

Nit: made in content -> made in web pages

("content" makes sense to Mozilla developers who understand the specific usage of that term in Mozilla, but it doesn't make as much sense to casual web developers.)


>+    The content to send to the server. If `content` is a string, it must be
>+    URL encoded (use `encodeURIComponent`). If `content` is an object, it should

Nit: URL encoded -> URL-encoded


>+<api name="statusText">
>+@property {string}
>+The HTTP response status line (e.g. *OK*)

Nit: period at end of sentence fragment.


>+## Examples
>+
>+### Getting the Most recent Public Tweet ###
>+
>+    var latestTweetRequest = Request({
>+      url: "http://api.twitter.com/1/statuses/public_timeline.json",
>+      onComplete: function () {
>+        var tweet = this.response.json[0];
>+        console.log("User: " + tweet.user.screen_name);
>+        console.log("Tweet: " + tweet.text);
>+      }
>+    });

Nit: this should then do:

  latestTweetRequest.get();

(Otherwise, it won't actually do what it suggests!)


>diff --git a/packages/jetpack-core/lib/request.js b/packages/jetpack-core/lib/request.js

>+      let index = h.indexOf(":");
>+      // The spec allows for leading spaces, so instead of assuming a single
>+      // leading space, just trim the values.
>+      let key = h.substring(0, index).trim(),
>+          val = h.substring(index + 1).trim()

Nit: semi-colon at end of line.


All issues are nits, r=myk!

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/labs/jetpack-sdk/rev/f9d244b8bdf4 with nits fixed. Thanks for the speedy review!

Comment 18

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".