-
Notifications
You must be signed in to change notification settings - Fork 40
Allow setting a maxAge #292
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
base: main
Are you sure you want to change the base?
Conversation
annevk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other fields we have some validation. Do we want to throw if both expires and maxAge are passed?
|
hmm i was planning to let that decision fall to the rfc and just prefer the |
|
Once we start targeting the layered-cookies I-D instead of the current RFC, I'm pretty sure we'll need to resolve the duplication locally anyway as we're not going to serialize what we have and feed it into the parser, but instead we'll create a cookie instance here and use that. |
|
Oh I didnt realize that, happy to check which is specified and throw an error in that case. Would you rather we construct a unified expiry time (between |
|
I think we should leave that until we update this specification to target the layered-cookies I-D. @bakulf thoughts on all this? |
DCtheTall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one suggestion
Add
maxAgeas a CookieInitOption used in cookieStore.set(options). Related to #57 #162I was inclined to use a
long longand allow the rfc to handle limiting the cookie ttl to 400 days if there is a restriction imposed there. Alongmight overflow a bit earlier than expected ~10 years and expire the cookie immediately but im not sure if protecting against that is actually needed.(See WHATWG Working Mode: Changes for more details.)
Preview | Diff