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

setCookie example in docs incorrectly handles expires and maxAge #28453

Closed
will-stone opened this issue Aug 24, 2021 · 3 comments · Fixed by #36870
Closed

setCookie example in docs incorrectly handles expires and maxAge #28453

will-stone opened this issue Aug 24, 2021 · 3 comments · Fixed by #36870
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@will-stone
Copy link

will-stone commented Aug 24, 2021

The example code for a setCookie function (https://nextjs.org/docs/api-routes/api-middlewares#extending-the-reqres-objects-with-typescript) shows the maxAge option being divided by 1000.

I believe the function should be:

if (typeof options.maxAge === 'number') {
  options.expires = new Date(Date.now() + options.maxAge * 1000);
}

The max-age attribute for Set-Cookie is in seconds. Therefore I believe there's a few changes that are required to the middle conditional:

  1. The check should only be concerned with whether the maxAge is numerical.
  2. As Date.now provides time in milliseconds, we need to multiply options.maxAge by 1000.
  3. options.maxAge does not require altering as it can be passed on in seconds to the serialize function which expects it in seconds:

Originally posted by @will-stone in #27617 (comment)

@balazsorban44
Copy link
Member

This is a good point in my opinion, that ties in with #36478 (comment)

@balazsorban44 balazsorban44 added the good first issue Easy to fix issues, good for newcomers label Apr 29, 2022
@ghost
Copy link

ghost commented May 12, 2022

Hi, this is my first open source contribution. I have raised a pull request for this issue:#36870 . Please review my changes and let my know any changes that I ought to do. Thanks

@kodiakhq kodiakhq bot closed this as completed in #36870 May 13, 2022
kodiakhq bot pushed a commit that referenced this issue May 13, 2022
fixes #28453

## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [X] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [X] Make sure the linting passes by running `yarn lint`
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants