natevw / node-chargify

Simple wrapper around Chargify's REST API for node.js
17 stars 12 forks source link

Rewrite to adopt node conventions #1

Closed willwhite closed 12 years ago

willwhite commented 12 years ago

Hi @natevm, I've taken a first stab at rewriting this module with the goal of making it's conventions more familiar to a node developer.

Summary of the changes:

Let me know if you have any feedback or opinions about this becoming a 0.2.x release of the project.

Thanks, Will

natevw commented 12 years ago

Uh-oh, this makes me feel bad!

These changes look very good overall, but I realize now I forgot to put any note in the documentation about what became of this library.

Basically, this library was sort of the "version 0.1" of Fermata — which now has a Chargify plugin that I recommend instead of this standalone version. There's nothing wrong per se with this library (especially with these improvements) but it makes more sense to me to support just one "REST wrapping" library that's pluggable for config-type stuff but shares a common JavaScript-side API across every service.

What do you think? I'm a little torn — I like a lot in this pull request, but it does change the API for anyone else who's using this. So from my perspective it doesn't seem wise to "break" what I consider a "legacy" API — but I'm open to whatever suggestions/feedback you've got.

(I really appreciate you taking the time to submit a pull request, and like I said I feel bad that the documentation does not make the state of this project more clear.)

willwhite commented 12 years ago

Hi @natevw, No worries! I've been using node-chargify for a bit now and actually didn't bother to do any searching around before sketching out this rewrite. Since the project is using semver, I don't think there would be a problem with breaking the API in a 0.2.x release. API users should not be anticipating a clean upgrade between these versions on the 0.x.x branch.

The plugin architecture of Fermata is interesting. I just prefer the Request module's API and I consider the approach on this branch almost as a "plugin" for the Request module.

I'm still interested in merging and supporting this branch, but I'm happy to discuss further.

Thanks, Will

natevw commented 12 years ago

Thanks, Will.

I agree that Request is a great module as well — I've actually used it alongside Fermata in projects. To me they fill different roles: Request is great when you have a "static URL" (or dynamically rewritten, as in a proxy) and still want streaming/buffering, while I designed Fermata specifically for dealing with "answers" to/from REST API calls — I just got really sick of concat'ing URL strings together by hand all over my code (especially in clientside CouchApps) and so Fermata emphasizes that side of things. @mikeal's library emphasizes the crazy cool node.js async streaming stuff.

Anyway, if semver considers 0.2.x incompatible with 0.1.x this should be reasonably safe to merge in and I plan to do so once I've had a chance to properly read through the diff. Thanks again!

mikeal commented 12 years ago

Streams are great but they are only useful when you're taking data from one file descriptor and handing it over, and possibly mutating it, to another.

The standard node conventions for "asking question" from IO, of which REST API calls would certainly qualify, is the standard callback function (err, result) {} as the last argument. APIs that don't use this interface will have a very hard time integrating in to the rest of the node ecosystem and being using in conjunction with other libraries.

Since you have a higher level abstraction than request (REST > HTTP) you can consider a lot more as an "error", including HTTP errors, where as request only considers socket errors to be "errors".

willwhite commented 12 years ago

I was torn on the decision to return certain HTTP codes (> 400, for instance) in err. For now I decided to just follow the Request API, but I'm happy to reconsider that. Good to hear your opinion on it. How would the API user be able to tell the difference between a HTTP level error vs. a socket error? In some cases that distinction is important to the user.

mikeal commented 12 years ago

It's all about where your layer is. I have a small CouchDB API I use on top of request and it's primary function is to turn non-2xx responses in to error :)

natevw commented 12 years ago

Merged in, thanks again!