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

Rewrite to adopt node conventions #1

Merged
merged 10 commits into from
May 5, 2012
Merged

Rewrite to adopt node conventions #1

merged 10 commits into from
May 5, 2012

Conversation

willwhite
Copy link
Collaborator

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:

  • Use the battle-tested Request module for HTTPS requests.
  • Callbacks accept err as their first argument, so connection level errors are reported properly.
  • Added mocha tests

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

Thanks,
Will

@natevw
Copy link
Owner

natevw commented May 2, 2012

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
Copy link
Collaborator Author

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
Copy link
Owner

natevw commented May 2, 2012

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
Copy link

mikeal commented May 2, 2012

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
Copy link
Collaborator Author

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
Copy link

mikeal commented May 2, 2012

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 natevw merged commit cc11ead into natevw:master May 5, 2012
@natevw
Copy link
Owner

natevw commented May 5, 2012

Merged in, thanks again!

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

Successfully merging this pull request may close these issues.

4 participants