Fix: maxAge description in readme

[likerRr]

There is a mistake in maxAge description - should be seconds instead of milliseconds

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[jsumners]

Can you point out where it is being interpreted as seconds? I understand the cookie spec defines it as seconds, but the implementation looks to be in milliseconds:

session/lib/cookie.js

Lines 42 to 46 in 25b4f6b

	set maxAge (ms) {
	this.expires = new Date(Date.now() + ms)
	// we force the same originalMaxAge to match old behavior
	this.originalMaxAge = ms
	}

[likerRr]

Nice catch! I was followed by the typings and they brought me here, which seems to be the issue with the typings, not with the readme. maxAge from @fastify/cookie is described like a value in seconds

[likerRr]

What are use cases setting maxAge in milliseconds? Seems like it's a common practice to set in seconds

[gurgunday]

Yeah, I think we should swap out this cookie module with the one from fastify/cookie

[likerRr]

But that would mean that major version should be increased, which doesn't seem like a significant change) So probably a fix to the typings would be more then enough

[gurgunday]

I see - can you point to where the issue is in types?

[jsumners]

What are use cases setting maxAge in milliseconds?

JavaScript's date handling is based upon milliseconds, not seconds.

[likerRr]

What are use cases setting maxAge in milliseconds?

JavaScript's date handling is based upon milliseconds, not seconds.

I meant that it's common to set the age for cookies in seconds. So from the dev side it's easier to think in this level of abstraction rather then how the language handles dates.

dismissed

[chuanqisun]

Hey @gurgunday (or whom it may concern), sorry for bringing up a stale issue again. I believe there is still an issue here. This is my most recent observation.

In the typing file, @fastify/session :: CookieOptions is already based on @fastify/cookie :: CookieSerializeOptions which interprets the value as seconds

@fastify/session/types/types.d.ts

import { CookieSerializeOptions } from "@fastify/cookie"
...
export interface CookieOptions extends Omit<CookieSerializeOptions, 'signed'> {}

This means when user hover over the property, they will believe that the unit is seconds, which contradicts the implementation.

Screenshot:

If we continue with the milliseconds implementation, more typescript users could build applications with the wrong assumption that the value is seconds. I see two options:

1. Fix the typing. Stop relying on @fastify/cookie's type. Something like this might work:

   export interface CookieOptions extends Omit<CookieSerializeOptions, 'signed' | 'maxAge'> {
      /** A `number` in milliseconds that specifies the `Expires` attribute by adding the specified seconds to the current date. If both `expires` and `maxAge` are set, then `expires` is used. */
     maxAge?: number;
   }

2. Fix the implementation. Make it a breaking change. Because @fastify/session already uses @fastify/cookies, it seems natural to adopt the same unit.

[gurgunday]

I think we should do both 😅

Can you first make a PR to main with the first option, and we can follow up with the second for the next branch

[chuanqisun]

I think we should do both 😅

Can you first make a PR to main with the first option, and we can follow up with the second for the next branch

See #245

[gurgunday]

Thanks for the try!