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

Add native support for cookie persistence handling using an API compatible with tough-cookie #566

Closed
gajus opened this issue Aug 16, 2018 · 4 comments
Assignees
Labels
enhancement This change will extend Got features

Comments

@gajus
Copy link
Contributor

gajus commented Aug 16, 2018

Adding native cookie support has been discussed in multiple issues, e.g.

However, none of the issues cover how one is supposed to implement cookie persistence between redirects.

It is simple to implement something like tough-cookie for a single request with no redirects. However, I haven't figured out how to keep track of cookies when a cookie is set on a redirect and the final destination depends on the cookie being present. It probably requires handling "response" and "redirect" events and modifying the request. Point is, as a consumer of an HTTP client, I want an in-built cookie support or at least an API and a recipe that allows to quickly add cookie support. Otherwise, the learning curve for starting to use Got becomes unreasonably high compared to the available alternatives, all of which (request, axios, superagent) support cookie persistence out of the box.

@gajus
Copy link
Contributor Author

gajus commented Aug 16, 2018

In terms of implementing cookie support:

You need to construct the initial request with a cookie:

const jar = configuration.jar;

if (jar) {
  httpClientConfiguration.headers = {
    ...httpClientConfiguration.headers,
    cookie: jar.getCookieStringSync(configuration.url)
  };
}

You need to handle recording the cookie upon receiving a response:

(This configuration assumes that you have {throwHttpErrors: false}.)

activeRequest
  .then((response) => {
    if (userConfiguration.jar) {
      const jar = userConfiguration.jar;

      if (response.headers['set-cookie']) {
        jar.setCookieSync(response.headers['set-cookie'], response.url);
      }
    }
  });

... and you need to register cookie upon a redirect:

const activeRequest = got(url, httpClientConfiguration);

if (userConfiguration.jar) {
  const jar = userConfiguration.jar;

  activeRequest.on('redirect', (response, nextConfiguration) => {
    if (response.headers['set-cookie']) {
      jar.setCookieSync(response.headers['set-cookie'], response.url);

      const cookie = jar.getCookieStringSync(nextConfiguration.href);

      if (cookie) {
        nextConfiguration.headers = {
          ...nextConfiguration.headers,
          cookie
        };
      }
    }
  });
}

It is not a whole lot of code, but everyone implementing this themselves is going to create a whole lot of bugs.

@gajus
Copy link
Contributor Author

gajus commented Aug 16, 2018

Looks like "set-cookie" can be an array.

if (response.headers['set-cookie']) {
  const headers = Array.isArray(response.headers['set-cookie']) ? response.headers['set-cookie'] : [response.headers['set-cookie']];

  for (const header of headers) {
    jar.setCookieSync(header, response.url);
  }
}

@sindresorhus
Copy link
Owner

Makes sense. I don't think we want to directly depend on touch-cookie as it's pretty big, but we could add a cookie option that expects a new tough.CookieJar() instance? Or just {cookie: require('touch-cookie')}, not sure which is better.

@sindresorhus sindresorhus added enhancement This change will extend Got features ✭ help wanted ✭ labels Aug 24, 2018
@szmarczak
Copy link
Collaborator

I'd prefer cookieStorage instead of cookie. cookie is confusing with headers.cookies.

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
Projects
None yet
Development

No branches or pull requests

3 participants