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

Fix for maxAge == 0 that doesn't appear in serialized data. #20

Merged
merged 2 commits into from
Feb 23, 2014
Merged

Fix for maxAge == 0 that doesn't appear in serialized data. #20

merged 2 commits into from
Feb 23, 2014

Conversation

tucan
Copy link
Contributor

@tucan tucan commented Feb 17, 2014

Hello!

I found and fixed one bug. Then I pass to serialize options like { maxAge: 0 } (in order to force browser to remove cookie) in output I does'n see Max-Age options. In other cases (positive or negative max ages) all are OK.

@defunctzombie
Copy link
Contributor

This would fail if maxage was an empty string ''.

Should probably use a check for a number. We could employ this trick:

if (!isNaN(maxAge - 0)) { }

And ignore the value (or throw, or use 0) if it isn't a number. In this case empty string will become 0.

@tucan
Copy link
Contributor Author

tucan commented Feb 19, 2014

I think decision to throw an exception will be the best in this case. It's standard way for serializing functions like JSON.stringify.

If you agree this approach, I will make another pull request.

@defunctzombie
Copy link
Contributor

Sure. We can throw if not a number.
On Feb 19, 2014 4:05 AM, "Vladimir Andreev" notifications@github.com
wrote:

I think decision to throw an exception will be the best in this case. It's
standard way for serializing functions like JSON.stringify.

If you agree this approach, I will make another pull request.

Reply to this email directly or view it on GitHubhttps://github.com//pull/20#issuecomment-35478494
.

@tucan
Copy link
Contributor Author

tucan commented Feb 23, 2014

I did it.

Approach is following:

  1. Check whether opt.maxAge isn't null or undefined
  2. If so try to convert it to Number
  3. If failed throw Error
  4. push value into pairs array

defunctzombie added a commit that referenced this pull request Feb 23, 2014
Fix setting maxAge to 0
@defunctzombie defunctzombie merged commit 191fcff into jshttp:master Feb 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants