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

Added plugin cookie jar for Issue #1028 #1039

Merged
merged 3 commits into from Jul 12, 2018

Conversation

Projects
None yet
2 participants
@coolhome
Contributor

coolhome commented Jul 9, 2018

Closes #1028

@welcome

This comment has been minimized.

welcome bot commented Jul 9, 2018

💖 Thanks for opening this pull request! 💖

To help make this a smooth process, please be sure you have first read the
contributing guidelines.

@coolhome coolhome changed the title from Added plugin cookie jar for PR #1028 to Added plugin cookie jar for Issue #1028 Jul 9, 2018

@gschier

This looks awesome @coolhome! Thanks for doing this. I just had one comment about the implementation.

// Validate cookie domain
let cookiesWithMatchedDomain = cookies.filter(cookie => cookie.domain === cookieDomain);
if(cookiesWithMatchedDomain.length === 0) {

This comment has been minimized.

@gschier

gschier Jul 9, 2018

Collaborator

What do you think of changing this to use the same matching logic as the Request => Cookie plugin? This would allow the user to also match on URL path and would keep the behavior consistent.

function getCookieValue(cookieJar, url, name) {
return new Promise((resolve, reject) => {
const jar = jarFromCookies(cookieJar.cookies);
jar.getCookies(url, {}, (err, cookies) => {
if (err) {
console.warn(`Failed to find cookie for ${url}`, err);
}
if (!cookies || cookies.length === 0) {
reject(new Error(`No cookies in stored for url "${url}"`));
}
const cookie = cookies.find(cookie => cookie.key === name);
if (!cookie) {
const names = cookies.map(c => `"${c.key}"`).join(',\n\t');
throw new Error(
`No cookie with name "${name}".\nChoices are [\n\t${names}\n] for url "${url}"`
);
} else {
resolve(cookie ? cookie.value : null);
}
});
});
}

This comment has been minimized.

@gschier

gschier Jul 9, 2018

Collaborator

Also note, this would mean changing the "Cookie Domain" parameter to accept a more flexible "URL"

This comment has been minimized.

@coolhome

coolhome Jul 9, 2018

Contributor

@gschier I did not realize that the request plugin did this. I am now using the same function.

:)

EDIT: I'm still not sure if "Url" is a good argument. I did add a little description with an example.

@gschier

Awesome work @coolhome!

@gschier gschier merged commit 0aea04d into getinsomnia:develop Jul 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

welcome bot commented Jul 12, 2018

Congrats on merging your first pull request! 🎉🎉🎉 You're helping make Insomnia awesome! 🙌

@coolhome coolhome deleted the coolhome:issue-1028 branch Jul 12, 2018

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