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 cookies.remove(name[, options]) method #98

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add cookies.remove(name[, options]) method #98

wants to merge 4 commits into from

Conversation

smcmurray
Copy link

An easy way to expire a cookie.

An easy way to expire a cookie.
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good idea to me. Looks like Travis CI says there is a syntax error with the code, if you can take a look. Also please add tests for the new feature when you get a chance!

@smcmurray
Copy link
Author

I think it doesn't like the options argument default.
@dougwilson, How important is it to support old js engines?

@dougwilson
Copy link
Contributor

Hi @smcmurray unless it is impossible to implement without new versions, then we want to support. If it's impossible to implement your feature on old Node.js versions, then we can hold the PR for sometime in the future in which we no longer support them 👍

@smcmurray
Copy link
Author

I'm not sure how to build a test with new cookies().
All the current tests use new cookies.Cookie(). But that doesn't allow me to test cookies.remove()

@dougwilson
Copy link
Contributor

Hi @smcmurray it's no problem. We have many tests using new Cookies() just by the nature we have 100% code coverage currently. If you're having trouble coming up with a test even after looking for the test code, I'll help out Monday and make a test for you and push it up to your pull request here, no problem at all 👍

@dougwilson dougwilson added this to the 0.8 milestone Sep 25, 2018
@dougwilson dougwilson removed this from the 0.8 milestone Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants