-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add cookieMaxAge and secureCookies options #1612
Conversation
🦋 Changeset is good to goLatest commit: 69a3196 We got this. Not sure what this means? Click here to learn what changesets are. |
Yeah, it's starting to get a bit crowded there. I'm thinking perhaps we have a One thing I realised about #1596 is the |
Yeah, I'm a big fan of that kind of grouping. Doesn't have to be on this PR, but something to consider for the future. |
29186ac
to
a5db0f8
Compare
a5db0f8
to
24aa382
Compare
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 👍
Resolves #1596 and #1595
I followed the pattern of adding options to the keystone object. Not sure if I should have grouped these under cookies or something, we're getting quite a few config options here and I'm not sure these should be on the Keystone constructor?
We could probably save the above for a more general discussion in future.
For both options I've set defaults for
dev
andproduction
- Can you check these are right?Can I also get some help on how best to test this? Commits welcome.