Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace tough-cookie #882

Closed
1 task done
Rokt33r opened this issue Sep 19, 2019 · 9 comments · Fixed by #921
Closed
1 task done

Replace tough-cookie #882

Rokt33r opened this issue Sep 19, 2019 · 9 comments · Fixed by #921
Labels
enhancement This change will extend Got features ✭ help wanted ✭

Comments

@Rokt33r
Copy link

Rokt33r commented Sep 19, 2019

What problem are you trying to solve?

I'm back from here. #811
It is an issue of tough-cookie library. But the fix is in stale status for several months...

Describe the feature

So I'm thinking it would be really nice if got provides an abstract interface so I can use other cookie store than tough-cookie.

Checklist

  • I have read the documentation and made sure this feature doesn't already exist.
@sindresorhus
Copy link
Owner

Out of curiosity, what other cookie storage would you use?

@szmarczak
Copy link
Collaborator

It is an issue of tough-cookie library. But the fix is in stale status for several months...

https://github.com/salesforce/tough-cookie/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+author%3ARokt33r

You haven't opened such issue. Not to mention that tough-cookie is RFC-compliant.

@tobenna
Copy link

tobenna commented Sep 19, 2019

I think @Rokt33r is referring to salesforce/tough-cookie#130

@szmarczak
Copy link
Collaborator

Ah. In this case I would just open a new PR. The fix is very simple.

@szmarczak
Copy link
Collaborator

szmarczak commented Sep 19, 2019

So I'm thinking it would be really nice if got provides an abstract interface so I can use other cookie store than tough-cookie.

You just need to pass an object having these and you're good to go:

  • setCookie(cookieOrString, currentUrl, [{options},] cb(err,cookie))
  • getCookieString(currentUrl, [{options},] cb(err,cookies))

const setCookie = options.cookieJar ? promisify(options.cookieJar.setCookie.bind(options.cookieJar)) : null;
const getCookieString = options.cookieJar ? promisify(options.cookieJar.getCookieString.bind(options.cookieJar)) : null;

@szmarczak szmarczak added the question The issue is a question about Got label Sep 19, 2019
@Rokt33r
Copy link
Author

Rokt33r commented Sep 23, 2019

@sindresorhus I'm not using any for now. But if got makes it possible, I would have options to fork tough-cookie or to make another one from the scratch.

@szmarczak I think we need to define cookieJar interface in this package too. Otherwise, I still need to install @types/tough-cookie every time.

Also it would be awesome if the new interface's methods return Promise rather than accepting callback although it should be breaking change...

@szmarczak
Copy link
Collaborator

I think we need to define cookieJar interface in this package too. Otherwise, I still need to install @types/tough-cookie every time.

👍

Also it would be awesome if the new interface's methods return Promise rather than accepting callback although it should be breaking change...

I agree. Maybe we'll see these promises in Got v11. Let's stay with the callbacks for now.

@sindresorhus
Copy link
Owner

@szmarczak It would be nice not to be so tied to tough-cookie. We could maybe support both though. Detect whether the input is tough.CookieJar and if not, have a nicer general interface using promises. ?

@szmarczak szmarczak added enhancement This change will extend Got features ✭ help wanted ✭ and removed question The issue is a question about Got labels Nov 4, 2019
@szmarczak
Copy link
Collaborator

have a nicer general interface using promises. ?

cookieJar needs to be an object:

interface CookieJar {
	getCookieString(url: string, callback: (error: Error, cookieHeader: string) => void);
	setCookie(rawCookie: string, url: string, callback: (error: Error, result: unknown) => void);
}

we just need to detect if it's a Promise or is it a callback-style function :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants