decouple things a bit #619

Merged
merged 4 commits into from Aug 7, 2013

Projects

None yet

3 participants

@joaojeronimo
Contributor

I was trying to make request "browserifyable" again, and I thought to myself it would be easier if I just used what I needed and took out everything else, which also probably causes problems in the "browserification". So I'm going to try to make a request fork that is more minimal and extendable, just like the ecosystem of leveldb modules that go on extending levelup.

I'm not going to make a pull request of this fork I'm making because it breaks request, but this is a checkpoint along that way, which I thought would be cool for request as well. It just decouples things a tiny bit, by putting components found in index.js in other files. It does not change the API and the tests are passing.

@mikeal
Member
mikeal commented Aug 7, 2013

please don't rename the file name to Request.js, it makes the patch really hard to figure out because all of the file changes are buried in a file move.

@joaojeronimo
Contributor

it looks a lot like a rename but it isn't one. Request.js exports the Request object, index.js requires this and deals with the higher level methods like .get, .post, ..., .jar. It makes it easier to break this file after into even smaller pieces, each of them extending the Request object.

@mikeal
Member
mikeal commented Aug 7, 2013

hrm.... can you put that back in index.js for this patch to make it easier to evaluate. then we can create a seperate patch that only moves the Request object.

@joaojeronimo
Contributor

This is the patch that only moves the Request object, and 3 other functions could be easily isolated (copy, debug and getSafe).

This other branch goes beyond moving the Request object and really puts most things out like authentication mechanisms, forms, multipart, etc. I'm not sure you would want that merged here though.

@mikeal
Member
mikeal commented Aug 7, 2013

ok then.

  • need to remove .DS_STORE from the patch
  • rename Request.js to lib/request.js

Then we can merge, provided all the tests pass.

@joaojeronimo
Contributor

I'm sorry I'm not used to macs and from time to time forget about those things. I think it's ok now.

@mikeal mikeal merged commit 9187a12 into request:master Aug 7, 2013
@mikeal
Member
mikeal commented Aug 7, 2013

ideally we could pull in a dependency to do the copying

@joaojeronimo
Contributor
var extend = require('util')._extend

function copy(obj) {
  return extend({}, obj)
}

?

@Floby

Is there a way to turn this off or override this behaviour ?
I understand this is most of the time what I need, but I've been encountering cases where it isn't. I couldn't find anything to change this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment