Skip to content
This repository has been archived by the owner on Aug 7, 2022. It is now read-only.

Add Promise compatibility #45

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

awesomejerry
Copy link

  • Update dependency should
  • Return a promise for then chaining

@awesomejerry
Copy link
Author

Any consideration to upgrade node version in travis?
0.10 is kind of legacy. If not, I'll try to add Promise polyfill.

@adunkman
Copy link
Owner

0.10 is kind of legacy.

Understatement of the year! 😄 Definitely, just merged #49, but feel free to upgrade support further if you need to.

Can you also include an update to the README? Thanks!

@awesomejerry
Copy link
Author

I've updated the README!

@dfdeagle47
Copy link

dfdeagle47 commented Oct 16, 2017

If I'm not mistaken, it doesn't exactly work as shown in the README.

It says this is valid

// Use it as a promise
t.get("/1/members/me", { cards: "open" })
  .then(function(data) { console.log(data); });

However, the params don't seem to be taken into account.

On the other hand, this works as intended

t.get("/1/members/me", { cards: "open" }, (err, data) => console.log(err, data))

My guess is, the problem comes from those lines https://github.com/awesomejerry/node-trello/blob/master/lib/node-trello.js#L50-L53.

 if (arguments.length === 3) {
    callback = argsOrCallback;
    args = {};
  }

You're checking if the function receives 3 arguments, the third argument is considered the callback. This is incorrect in the first case: it calls the function with the arguments
("GET", "/1/members/me", { cards: "open" })

I suppose changing it to
arguments.length === 4
should work as intended.

EDIT: On second thought, this might not work either if the function is called with

t.get("/1/members/me", (err, data) => console.log(err, data))

Because it will indeed produce the 3 arguments
("GET", "/1/members/me", (err, data) => console.log(err, data))

EDIT2: maybe this would work https://github.com/dfdeagle47/node-trello/blob/master/lib/node-trello.js#L53-L60

Cheers

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants