-
Notifications
You must be signed in to change notification settings - Fork 9
fix: update getExpirationDate so touch applies #87
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: master
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR addresses issue #82 by updating the behavior of getExpirationDate in the session store to ensure that the session touch operation properly applies the expiration reset. The key changes include:
- Reordering of imports for clarity.
- Updated getExpirationDate implementation that prioritizes sessionCookie.expires if it's a Date, falls back to sessionCookie.maxAge, and otherwise uses the default TTL.
Comments suppressed due to low confidence (1)
src/store.ts:1
- [nitpick] Consider verifying that the updated import order adheres to your project's ordering guidelines for consistency across the codebase.
import session, { Store as ExpressSessionStore, SessionData } from 'express-session';
bradtaniguchi
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.
At a glance the change seems sensible.
I dislike the original implementation's use of an OR leading directly into a ternary as that's somewhat confusing (I got tripped up as I wasn't completely sure if the OR takes precedent over the rest of the ternary)
I'll have to review the docs on the provided cookie and maybe cross check this sort of change with other session stores.
For the most part, pretty sane changes from AI here.
JLuboff
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.
Looks okay to me, and is passing tests locally...
|
So I skimmed the express-session docs here: I've had a busy weekend, so will go over this more in-depth. The docs also gave one example, the redis-store, so I checked out their implementation as a reference: Will re-read both sometime this week. |
Fix: #82