Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API is not nodeback-compatible #26

Open
joepie91 opened this issue Aug 7, 2015 · 6 comments
Open

API is not nodeback-compatible #26

joepie91 opened this issue Aug 7, 2015 · 6 comments

Comments

@joepie91
Copy link

joepie91 commented Aug 7, 2015

Currently, the typical function signature in this module looks like this:

doThing(someArgument, function(err, response, body) { ... }, function(result) { ... })

... however, to conform to the nodeback (Node.js callback) convention, it should look like this:

doThing(someArgument, function(err, result) { ... })

... where err is an Error object (not a string!), and optionally has response and body on it as additional properties (like this Instagram client does).

In the current situation, the API is confusing as it's not consistent with other asynchronous libraries, and it's not possible to use third-party tools with it that expect nodebacks - for example, you can't promisify this library.

Changing the API would be a breaking change, and thus require incrementing the major version number.

@rakannimer
Copy link
Contributor

Very good point. To do this we just need to update the doPost and doRequest functions. Are you interested in submitting a PR ?

@joepie91
Copy link
Author

joepie91 commented Aug 7, 2015

Well, you'd also need to update the external API methods (getSearch, postTweet, getUserTimeline, ...), and the docs, as the function signature for those would change.

I don't think I'm in a very good position to submit a PR, though - I haven't used this module myself and don't have a testing environment (or OAuth key, for that matter) here. I simply came here to file an issue because somebody I know got stuck on the API, and I noticed the issue :) So it'd probably be better if somebody more familiar with the codebase and API made these changes.

@rakannimer
Copy link
Contributor

Yeah I see what you mean. I'll open up a new branch on my fork and get on it when I have time. I often use bluebird promises and it would be nice to be able to use it with this library :)

@joepie91
Copy link
Author

joepie91 commented Aug 7, 2015

Another possibility to look into, would be to use Bluebird internally and to export the API using nodeify. That will give you a dual promises and nodeback API, and leave it up to the user which to use.

@BoyCook
Copy link
Owner

BoyCook commented Sep 7, 2015

Is there any documentation on the nodeback convention?

@joepie91
Copy link
Author

joepie91 commented Sep 7, 2015

@BoyCook Not sure if there's formal documentation, but it's just a Node.js-style callback:

  • Always called asynchronously, never synchronously.
  • Signature is (err, result).
  • If there's an error, then err contains the Error object (not a string!); otherwise, it is null or undefined.
  • If there is no error, then result contains the result.

If you were to use Bluebird, then nodeify will certainly export a valid nodeback API :)

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

No branches or pull requests

3 participants