Gozala / reducers

Library for higher-order manipulation of collections
MIT License
181 stars 8 forks source link

Simplified http lib and removed url library dependency. #7

Closed gordonbrander closed 11 years ago

gordonbrander commented 12 years ago

This pull request contains 2 changes:

It removes the dependency on Node's url library.

Since this module is explicitly for browsers, I think it makes sense to avoid dependencies on Node core libraries. This is not a hard and fast rule, but more a rule of thumb.

Including Node dependencies means you must run the library through browserify. If we avoid dependencies on Node, it's possible to wrap the library in AMD or simply avoid modules altogether with minimal effort. Essentially, it's easier to maintain forks of different module flavors.

This seems a worthwhile goal to me. Some folks don't know how to use tools like browserify, others are stuck with a tool chain that prohibits it, others prefer AMD, etc.

Additionally, the cost of removing this dependency is low-to-nothing. The url library was being used to build URL strings from options objects. However, the url fragments it copied to the instance could not be counted on, since they would only appear on the Request instance if you had specified the URL via an object. If you specified the URL via a string, these properties would not appear. This made the interface for Request inconsistent.

Specifying URL with string is both the simpler and the more common case. It also removes the url lib dependency.

It changes uri to url.

This is stylistic, but I think it's helpful. URI is easily confused with URL because i, I, and l are easily misread. Since url is the more common term, I think it makes sense to use it.

For prior art, see jQuery.ajax, which uses url as a string property for requests.

I didn't see a test suite for http, so no tests have been added. Happy to write some if you like. I did smoke test the changes by hand.

Thanks!

travisbot commented 12 years ago

This pull request passes (merged 79d3fa2f into ed9263dc).

Raynos commented 11 years ago

uri is used for a good reason.

It's used in request and it doesn't conflict with the local variable url from require("url")

Gozala commented 11 years ago

This has moved to gozala/http-reduce