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

Don't automatically add Expires in res.cookie if expires option is explicitly set to 0 #5151

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nbkhope
Copy link

@nbkhope nbkhope commented Mar 19, 2023

This addresses #5150

Those who wish to exclude Expires (but still keep Max-Age from the final Set-Cookie statement cannot do so if maxAge option is passed to res.cookie).

This proposed solution allows one to exclude Expires if expires: 0 option is passed to res.cookie.

Comment on lines -875 to +877
opts.expires = new Date(Date.now() + maxAge)
if (opts.expires !== 0) {
opts.expires = new Date(Date.now() + maxAge)
}
Copy link

Choose a reason for hiding this comment

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

Allowing opts.expires to be of type number breaks the consistency, that in all other cases expires is of type Date. This is also enfored by the TS' type defintion @types/express as defined here.

Based on my comment on #5150 (comment), I would propose to remove opts.expires = new Date(Date.now() + maxAge) entirely. Developers can still set this value as part of the function call. The maintainers will need to decide if this would be a breaking-change.

Copy link
Author

Choose a reason for hiding this comment

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

@tjarbo thank you your feedback. I put zero because the docs said:

expires | Date | Expiry date of the cookie in GMT. If not specified or set to 0, creates a session cookie.

Ref https://github.com/expressjs/expressjs.com/blob/867a10d748c4f5c83dfdbc634e210621aa1a34a9/_includes/api/en/4x/res-cookie.md

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.

3 participants