criso / fbgraph

NodeJs module to access the facebook graph api
http://criso.github.io/fbgraph/
1.09k stars 176 forks source link

Pass postData on from del() to post() #25

Closed bergman closed 11 years ago

bergman commented 11 years ago

The way we're using this library is without ever logging in the client but instead passing the access token along with every request. This makes that work for DELETE requests.

criso commented 11 years ago

Awesome! But this will break backwards compatibility. Set it up so that postdata is optional similar to:

function Graph(method, url, postData, callback) {
  if (typeof callback === 'undefined') {
    callback  = postData;
    postData  = {};
  }

Also, please make sure that this won't break any tests and add a test for your use case.

bergman commented 11 years ago

No, I believe it behaves just as before. In fact, your suggestion would break backwards compatibility (it would pass an empty postData object to .post() which in turn will disable the inclusion of the access_token that .post() puts in there). I'm not sure what the correct way of handling this is. My first attempt was actually your suggestion but with the inclusion of access_token, just as in .post() but that seemed even worse because you would have to maintain two places if you ever decided to put some other field in there. Perhaps the extend() method should be used to merge the default postData with the one passed in, with preference for the one passed in.

I cannot get the tests to pass even before this change, it fails on this step:

With test users after creating user 1 and user2 and after a friend request has been made  - a post on user1's wall

Full test run here: https://gist.github.com/bergman/75129615b15f4ec1aab5

My facebook app is in sandbox mode if that matters.

Once I get the tests running I'll make a test case for this.

criso commented 11 years ago

I meant it as overloading the function. This should work:

// Both of these *need* to work
// del('/url', function(){})
// del('/url', {data: foo}, function() {})

exports.del = function (url, postData, callback) {

    if (typeof callback == 'function') {
        this.post(url, postData, callback); 
    } else {
        callback = postData;
        this.post(url, callback);
    }
};

The tests might be failing due to facebook. Ideally the api responses would've been stubbed, but I don't trust Facebook enough to think their api won't change all the time. Might be a legit failure due to api changes as well. I can't really look into it right now.