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

Require cookie value to be string (unset omitted or null) #114

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

Conversation

zachasme
Copy link

@zachasme zachasme commented Sep 11, 2019

Resolves #101.

This enforces string input as suggested by @dougwilson.

Two considerations:

  • Is input a string if wrapped in String constructor? I suggest no, as do Node. That is also what I went with in the PR.
  • When is input omitted? Logically undefined. But there are tests for expiring cookies using null. What about false? I feel undefined and null should cover it (an explicit Cookies.remove would avoid ambiguity, see Add cookies.remove(name[, options]) method #98).

@dougwilson dougwilson added the pr label Oct 9, 2019
@dougwilson
Copy link
Contributor

Is input a string if wrapped in String constructor? I suggest no, as do Node. That is also what I went with in the PR.

Yea, I have just done the same type of validations that Node.js APIs use, which is just typeof, only considering primitive types and not the boxed types.

When is input omitted? Logically undefined. But there are tests for expiring cookies using null. What about false? I feel undefined and null should cover it

From a JavaScript POV, it is just undefined. The two tests passing in null, are testing that any falsy value works, so that would mean the API contract there does indeed false and 0.

@zachasme
Copy link
Author

Would you prefer only considering input as omitted on undefined then? If so I can quickly update the PR accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookie set to '' when pass the value as 0
2 participants