Last Comment Bug 503116 - Twitter library improvements
: Twitter library improvements
Status: RESOLVED FIXED
:
Product: Mozilla Labs
Classification: Other
Component: Jetpack Prototype (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 0.5
Assigned To: Drew Willcoxon :adw
:
:
Mentors:
http://apiwiki.twitter.com/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-08 10:22 PDT by Drew Willcoxon :adw
Modified: 2009-07-24 14:12 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (14.82 KB, patch)
2009-07-12 16:48 PDT, Drew Willcoxon :adw
no flags Details | Diff | Splinter Review
patch v2 (26.93 KB, patch)
2009-07-21 23:49 PDT, Drew Willcoxon :adw
edilee: review+
Details | Diff | Splinter Review
patch v3 (26.45 KB, patch)
2009-07-24 12:40 PDT, Drew Willcoxon :adw
edilee: review+
Details | Diff | Splinter Review
v2 -> v3 interdiff (6.76 KB, application/octet-stream)
2009-07-24 12:51 PDT, Ed Lee :Mardak
no flags Details
patch v4 (24.23 KB, patch)
2009-07-24 14:09 PDT, Drew Willcoxon :adw
no flags Details | Diff | Splinter Review

Description Drew Willcoxon :adw 2009-07-08 10:22:43 PDT
Jetpack's Twitter library kind of sucks in at least the following ways:

1) You have to give a password to tweet.  The library should leave authentication to the browser.
2) It's not very comprehensive.
3) Is the separation of jetpack.lib.twitter.Twit -- a prototype representing a Twitter user -- and jetpack.lib.twitter -- "static" methods -- useful?  Needed?  Take a look at this at least.  The vast majority of Twitter API calls operate on a user.
4) Instead of always returning Twitter's response as is, wrap the response with our API.  For example, the various methods that return users, e.g., getting a user's friends, should return user objects, not a JSON'ed object.
5) It's not written in a Jetpacky style.
Comment 1 Drew Willcoxon :adw 2009-07-12 16:48:51 PDT
Created attachment 388169 [details] [diff] [review]
patch v1

Aza, what do you think about this approach to the API?  It's a really really thin, simple, KISS layer on top of Twitter's REST API.  No Twit or User objects, nothing fancy or silly.  It exposes each RESTful API method as a method on jetpack.lib.twitter, and each takes all its args as a single object:

// This first call will pop up Firefox's username and
// password dialog.  The user can tell Firefox to remember
// him, as when tweeting from the web as usual.
jetpack.lib.twitter.statuses.update({
  status: "Here's my tweet.",
  success: function (data) ...
  error: function (status, err) ...
});
jetpack.lib.twitter.statuses.public_timeline({
  success: function (data) ...
});
jetpack.lib.twitter.search({
  q: "search string",
  page: 2,
  since_id: 123456,
  success: function (data) ...
});

This makes it really easy to implement -- less than 200 LOC for Twitter's entire API.  And we can point people to Twitter's API wiki for most of the documentation.  And if you've already used the API, you'll feel right at home.

If it's OK with you, I'll ask Edward for a code review.  I'll need to attach a new patch that deprecates the current lib.twitter API.
Comment 2 Drew Willcoxon :adw 2009-07-21 23:49:16 PDT
Created attachment 389878 [details] [diff] [review]
patch v2

The Twitter lib is pretty sad, would be nice if this made the next release.
Comment 3 Drew Willcoxon :adw 2009-07-24 11:34:16 PDT
Comment on attachment 389878 [details] [diff] [review]
patch v2

Will post new patch that fixes the "favorites/get" vs. "favorites/create" cases by attaching functions to functions.  D'oh!
Comment 4 Drew Willcoxon :adw 2009-07-24 12:40:41 PDT
Created attachment 390517 [details] [diff] [review]
patch v3
Comment 5 Ed Lee :Mardak 2009-07-24 12:48:34 PDT
Comment on attachment 389878 [details] [diff] [review]
patch v2

>+++ b/extension/content/js/jetpack-environment.js
>@@ -152,17 +152,17 @@ window.addLazyLoaders(
>    "js/twitter.js": [
>-     "Twitter"
>+     "twitter"
Any particular reason for the case change?

>@@ -209,17 +209,17 @@ JetpackEnv.addImporter(
>   {"jetpack.lib.twitter": function(context) {
>-     return Twitter;
>+     return twitter;
This didn't apply to latest trunk. But we'll probably want to do "return twitter();" and remove the immediate function call from twitter.js. Because right now this returns a shared global to all contexts.

So a feature could do..

jetpack.lib.twitter.friends = "these aren't the friends you're looking for!";

and that will just break other features that use it. There's also the possibility of snooping other feature's call to twitter, but if you're a feature, you can read out any changes on the twitter server anyway..

>+++ b/extension/content/js/twitter.js
>+// There's one wrinkle.  Some Twitter methods are prefixes of others.
Already discussed in person -- attach functions to functions for js fun! :) Should automatically work if you just get rid of the /get. Perhaps note that that path entry needs to come first.

>+    addApi: function twitter_addApi(aApiDesc) {
>+      for (let [schemeAndHost, httpMethods] in Iterator(aApiDesc))
>+        for (let [httpMethod, paths] in Iterator(httpMethods))
>+          for (let i = 0; i < paths.length; i++)
>+            addApiMethod(this, schemeAndHost, paths[i], httpMethod);
For the innermost loop you could do paths.forEach(function(path) addApiMethod(twitter, schemeAndHost, path, httpMethod)

>+  function addApiMethod(aDestObj, aSchemeAndHost, aPath, aHttpMethod) {
..
>+      interpParam = pathFrags.length > 0 && pathFrags[0][0] === "$" ?
>+                    pathFrags.shift().substr(1) :
>+                    null;
Probably comment what we're looking for here.. see if the next fragment is something to interpret (begins with $). I know you have it commented above this whole section, but useful to have here too

Also, we only support one interpParam.. and that's good enough for now. I'm not familiar with twitter stuff, but any idea if there might be use for multiple things to interp? (No need to fix this now.)

Additionally, it's possible for the interp value to be "../../" or some uri changing.. but from some quick tests, if you have too many ../, it will cap out at the root of the domain. So nothing really bad I suppose. Just something to think about ;)

>+    obj[pathFrag] = function (opts) {
>+      var ajaxOpts = userOptsToAjaxOpts(opts);
>+      var url = [aSchemeAndHost].
>+                concat(urlFrags).
>+                concat(pathFrag !== "get" ? pathFrag : []).
>+                concat(interpParam ? ajaxOpts.data[interpParam] : []).
>+                join("/") + ".json";
You should be able to do this with one concat. [scheme].concat(url, path, interp);

>+  function userOptsToAjaxOpts(aUserOpts) {
>+    var dupe = {};
>+    for (let [prop, val] in Iterator(aUserOpts))
>+      dupe[prop] = val;
If we support opts that are objects down the line.. we'll might need to deep copy them instead of just copying the reference. So this includes objects, arrays, functions... And we do copy success/error functions already..

The problem only arises if we change properties on the ajax opts objects because then it'll change the property of the passed-in object as well.

>+    return "data" in dupe ? dupe : {
>+      data: dupe,
>+      success: let (s = dupe.success) delete dupe.success && s,
>+      error: let (e = dupe.error) delete dupe.error && e
You might be getting too fancy here! ;) But I don't have anything better that's as short... :p

>+  // DEPRECATED ///////////////////////////////////////////////////////////////
Atul seems to suggest one version is enough to keep deprecated stuff around, but we should discuss it on the forum or at the meeting.

>+  return twitter;
> }();
As mentioned above.. perhaps remove the () so that we can make it per context.

r=Mardak
Comment 6 Ed Lee :Mardak 2009-07-24 12:51:12 PDT
Created attachment 390519 [details]
v2 -> v3 interdiff
Comment 7 Ed Lee :Mardak 2009-07-24 12:58:34 PDT
Comment on attachment 390517 [details] [diff] [review]
patch v3

>+++ b/extension/content/js/twitter.js
>+    // If there was already an object at the end of the hierarchy (which may
>+    // happen if we're not careful with the ordering of our API description in
>+    // cases like "favorites" and "favorites/create"), reattach that object's
>+    // properties to the new method.
>+    for (let [prop, val] in Iterator(currEndObj || {}))
>+      obj[pathFrag][prop] = val;
I guess that works too ;)

if (currEndObj != null)
  for ..
    obj[path..

But I suppose you're going to do currEndObj !== undefined ;) Fine just leave it as || {} :P
Comment 8 Ed Lee :Mardak 2009-07-24 12:59:57 PDT
Oh ! I wonder..

obj[pathFrag].__proto__ = currEndObj ?

.. maybe we're getting too creative..
Comment 9 Drew Willcoxon :adw 2009-07-24 13:23:06 PDT
(In reply to comment #5)
> (From update of attachment 389878 [details] [diff] [review])
> >+++ b/extension/content/js/jetpack-environment.js
> >@@ -152,17 +152,17 @@ window.addLazyLoaders(
> >    "js/twitter.js": [
> >-     "Twitter"
> >+     "twitter"
> Any particular reason for the case change?

Just using the convention that uppercase indicates a constructor and lowercase a singleton.  I'll remove the immediate invocation and invoke the function per feature.

> So a feature could do..
> 
> jetpack.lib.twitter.friends = "these aren't the friends you're looking for!";
> 
> and that will just break other features that use it. There's also the

Good point.

> >+    addApi: function twitter_addApi(aApiDesc) {
> >+      for (let [schemeAndHost, httpMethods] in Iterator(aApiDesc))
> >+        for (let [httpMethod, paths] in Iterator(httpMethods))
> >+          for (let i = 0; i < paths.length; i++)
> >+            addApiMethod(this, schemeAndHost, paths[i], httpMethod);
> For the innermost loop you could do paths.forEach(function(path)
> addApiMethod(twitter, schemeAndHost, path, httpMethod)

Indeed I could. :)

> Also, we only support one interpParam.. and that's good enough for now. I'm not
> familiar with twitter stuff, but any idea if there might be use for multiple
> things to interp? (No need to fix this now.)

Over-generalization Evil.  Actually I was thinking of extending this simple approach to supporting RESTful APIs to other services like Flickr and Delicious, so it's something to think about.

> Additionally, it's possible for the interp value to be "../../" or some uri
> changing.. but from some quick tests, if you have too many ../, it will cap out
> at the root of the domain. So nothing really bad I suppose. Just something to
> think about ;)

The interp var is looked up in the caller's options object, so if the interp var is "..", the caller would have to pass in { "..": ??? }.  If the interp var is innocent, like "id", the caller can of course pass in { id: ".." }, but garbage in, garbage out.  Twitter will fail his XHR, that's all.  Anybody can add a Twitter API method via lib.twitter.addApi(), but again, garbage in, garbage out, no big deal, and I'll fix it so you screw up only your twitter instance.

> If we support opts that are objects down the line.. we'll might need to deep
> copy them instead of just copying the reference. So this includes objects,
> arrays, functions... And we do copy success/error functions already..

Over-generalization Evil.

> You might be getting too fancy here! ;) But I don't have anything better that's
> as short... :p

That's quite a compliment coming from Mardak! :D

(In reply to comment #8)
> .. maybe we're getting too creative..

\o/ :D
Comment 10 Drew Willcoxon :adw 2009-07-24 14:09:29 PDT
Created attachment 390542 [details] [diff] [review]
patch v4
Comment 11 Drew Willcoxon :adw 2009-07-24 14:12:45 PDT
http://hg.mozilla.org/labs/jetpack/rev/79e19eef38d0

Note You need to log in before you can comment on or make changes to this bug.