Implemented cookies - closes issue 82: https://github.com/mikeal/request/issues/82 #102

Merged
merged 12 commits into from Nov 14, 2011

Projects

None yet

2 participants

@alessioalex

Implemented cookies - closes issue 82: #82

I've also added tests and updated the README file.

@alessioalex alessioalex commented on an outdated diff Nov 12, 2011
main.js
@@ -406,6 +421,15 @@ Request.prototype.request = function () {
response.body = JSON.parse(response.body)
} catch (e) {}
}
+ if (response.statusCode == 200) {
+ // only add cookies if no custom jar is defined (by the user)
@alessioalex
alessioalex Nov 12, 2011

No need to 'remember' the cookie if the user wants to go 'stealth' - by setting the jar option

@alessioalex

Some things I forgot to mention:

@mikeal mikeal and 1 other commented on an outdated diff Nov 13, 2011
main.js
@@ -137,6 +140,18 @@ Request.prototype.request = function () {
setHost = true
}
+ // if user tries to define a bad cookie jar
@mikeal
mikeal Nov 13, 2011 Member

instanceof checks are harmful and unnecessarily restrictive.

assume people are satisfying enough of the contract with the object they pass in, failure will be obvious if they don't.

@alessioalex
alessioalex Nov 13, 2011

that's right, thing will fail anyway so no need for that

@mikeal
Member
mikeal commented Nov 13, 2011

i am slightly confused as to the usage of the jar option.

in my original api i had assume that the jar would hold the state of any cookies set. if no jar was passed (which is default) then cookie support would be off.

i see a few checks where jar not being set actually turns on cookie handling which is confusing so i would like a little clarification.

i'm also wondering if this jar implementation handles cookies set for different domains and is domain aware? if not, that's not the end of the world so long as someone can add support later.

@mikeal
Member
mikeal commented Nov 13, 2011

i see the support for domains in the cookie implementation, nice.

i also like the request.cookie() and jar.add(cookie) stuff, very nice.

still confused though why setting the jar stops cookies from being set in it.

@alessioalex

Well I thought cookies should be enabled by default (I thought people expect request to behave like the browser here) and if they really don't want cookies (or they want specific cookies) the default option should help with that.

If you want I can change this to be the other way around, I just thought it would be more helpful like this.

@mikeal
Member
mikeal commented Nov 13, 2011

mainly, i think that if self.jar is set that we should still set new cookiies on it.

you might want to keep two jars around for different requests, so you'll want them to be set by the setcookie header calls. if someone wants to only use cookies they set explicitly they could just override the add() function in their jar object.

i'm conflicted about using a global jar by default. i guess it's fine, but setting jar:false should work to disable cookie handling.

@alessioalex

Should be as you want now.

@mikeal
Member
mikeal commented Nov 14, 2011

i'm sick right now so i don't have a lot of time to review and test this.

i'm going to merge it but hold off on pushing a new release to npm for a few days until i feel better and can sit down with it.

thanks for the patch, i've been wanting this for a while.

@mikeal mikeal merged commit 43cd887 into request:master Nov 14, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment