Use tough-cookie CookieJar sync API #767

merged 2 commits into from Jan 14, 2014


None yet

3 participants

stash commented Jan 13, 2014

This restores backwards-compatibility with jar parameters (i.e. jar.setCookie takes two parameters and is guaranteed synchronous).

The async API for tough-cookie can't be used without major changes to request itself, so I've left that for another day.

stash commented Jan 13, 2014
@stash stash and 1 other commented on an outdated diff Jan 13, 2014
var addCookie = function (cookie) {
- if (self._jar){
- var targetCookieJar = self._jar.setCookie?self._jar:cookieJar;
- //set the cookie if it's domain in the href's domain.
- targetCookieJar.setCookie(cookie, self.uri.href, function(err){
- if (err){
- console.warn('set cookie failed,'+ err)
- }
- })
+ //set the cookie if it's domain in the href's domain.
+ try {
+ targetCookieJar.setCookie(cookie, self.uri.href);
+ } catch (e) {
+ self.emit('cookieError', e);
stash Jan 13, 2014 Contributor

not sure if this is better than the console.warn - let me know.

mikeal Jan 13, 2014 Member

when/where would this error?

you might want to just emit an error normally.

stash Jan 14, 2014 Contributor

The real-world failures should be quite limited; egregious formatting errors for the cookie or the domain. I'll change this to .emit('error')

@stash stash commented on the diff Jan 13, 2014
@@ -21,9 +21,8 @@ var optional = require('./lib/optional')
, ForeverAgent = require('forever-agent')
, FormData = optional('form-data')
- , Cookie = optional('tough-cookie')
- , CookieJar = Cookie && Cookie.CookieJar
- , cookieJar = CookieJar && new CookieJar
+ , cookies = require('./lib/cookies')
+ , globalCookieJar = cookies.jar()
stash Jan 13, 2014 Contributor

I think this name is more descriptive; i can change it back if you disagree.

@stash stash referenced this pull request Jan 13, 2014

setCookie is broken #743


@stash this definitely makes it more friendly. I tried to build a cleaner interface for using tough-cookie here: (needs to be refactored to use sync api now). What are your thoughts?

On a side note I have some ideas for implementing sync differently, but I'll post that on goinstant/node-cookie instead of here.

stash commented Jan 14, 2014

@lalitkapoor definitely take what I've got in this branch's lib/cookies.js - plus I like the flexible API you made.

stash commented Jan 14, 2014

@mikeal ok, changed that line to emit('error') instead of 'cookieError'

@mikeal mikeal merged commit b1224af into request:master Jan 14, 2014
@stash stash deleted the stash:stash/sync-cookie-api branch Jan 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment