Migrated to cookies #13

Merged
merged 4 commits into from Mar 4, 2012

Conversation

Projects
None yet
7 participants

oesmith commented May 6, 2011

As mentioned on the discussion thread for issue #8, here's my small patch to switch to cookies.

Cheers!

Olly

@oesmith oesmith Switch to cookies library (from cookie-node).
Switch to oauth 1.0A, add option to specify callback URL.
292980f

@jdub jdub commented on an outdated diff May 7, 2011

lib/twitter.js
*/
-Twitter.prototype.cookie = function(req) {
+Twitter.prototype.cookie = function(cookies) {
@jdub

jdub May 7, 2011

Owner

This API change makes it less of a drop-in replacement. If you can work around this, I'll pull this in as a medium-term change (as the OAuth web helper stuff will eventually move into another module).

Owner

jdub commented May 7, 2011

Thanks heaps! If we can avoid the API changes, I'll pull this in straight away. :-)

@jdub jdub commented on the diff May 7, 2011

lib/twitter.js
this.oauth = new oauth.OAuth(
this.options.request_token_url,
this.options.access_token_url,
this.options.consumer_key,
this.options.consumer_secret,
- '1.0', null, 'HMAC-SHA1', null,
+ '1.0A',
+ this.options.callback_url,
@jdub

jdub May 7, 2011

Owner

Good call. :-)

@oesmith oesmith Refactor cookie helper back to its original signature to avoid API br…
…eakage (and hide the cookie implementation details - nobody needs to see that ;)).
1717917

oesmith closed this May 7, 2011

oesmith commented May 7, 2011

Thanks lots for the feedback - much appreciated.

I've refactored to put the API back to how it was - all in the latest commit.

Cheers!

Olly

oesmith reopened this May 7, 2011

oesmith commented May 7, 2011

Whoops.. didn't mean to hit 'Close'..

@jdub jdub and 1 other commented on an outdated diff May 8, 2011

lib/twitter.js
@@ -248,16 +255,11 @@ Twitter.prototype.stream = function(method, params, callback) {
/*
* TWITTER "O"AUTHENTICATION UTILITIES, INCLUDING THE GREAT
* CONNECT/STACK STYLE TWITTER "O"AUTHENTICATION MIDDLEWARE
- * and helpful utilities to retrieve the twauth cookie etc.
@jdub

jdub May 8, 2011

Owner

This should not be removed.

@oesmith

oesmith May 8, 2011

Have re-instated this with latest commit.

@jdub jdub commented on the diff May 8, 2011

lib/twitter.js
@@ -291,7 +289,8 @@ Twitter.prototype.login = function(mount, success) {
}
// Fetch the cookie
- var twauth = self.cookie(req);
@jdub

jdub May 8, 2011

Owner

No need to replace this with the return of self.cookie()

@oesmith

oesmith May 8, 2011

'cookies' needs to be in scope lower down to set new cookie values.

I'm not sure if there's an more elegant way to do this with calling self.cookie(), without changing its signature. What do you think?

@jdub

jdub May 8, 2011

Owner

Ah yes, you need res to set... luckily, signature matters not. :-)

If prototype.cookie took (req, res), and passed res || null to the Cookies constructor, we'd be fine.

OK with that? Thanks!

@oesmith

oesmith May 8, 2011

We still need to Cookies object though.. how would you get that back from prototype.cookie?

fat commented Aug 2, 2011

what's the status of this... why not merge? Getting this message is disconcerting:

This library has been deprecated. Please use `npm install cookies` instead.

oesmith commented Aug 2, 2011

fwiw, I've been using this patch in production on http://tweetgp.com for the last 3 months with no issues.

same message as fat... please update

aseemk commented Aug 12, 2011

+1

slajax commented Sep 21, 2011

+2

is this fork totally abandoned?

oesmith commented Oct 9, 2011

I'm still using my fork in production.

Whether or not the changes will get pulled back into jdub's repo is another matter though..

@jdub jdub added a commit that referenced this pull request Mar 4, 2012

@jdub jdub Merge pull request #13 from oesmith/master
Migrated to cookies
eeede3a

@jdub jdub merged commit eeede3a into jdub:master Mar 4, 2012

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