-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: allows UPS to receive cookie config options #261
Changes from 5 commits
517d030
4399d6d
7133cbf
4eb37d4
3d1c139
ce59d6f
0a64297
3497173
6d2277d
1185884
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,23 +12,31 @@ export function getObject(key: string): string | null { | |
return value ? JSON.parse(value) : value | ||
} | ||
|
||
export function put(key: string, value: string | null, options: any /* TODO fix any */ = {}): void { | ||
export type CookieOptions = { | ||
path?: string | ||
domain?: string | ||
expires?: string | Date | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: string is for iso format? maybe restricting to Date only would make it clear for the consumers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree. Yeah string is for iso format cause the code where this came from originally supported both. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually made it only support string given that's what the cookie expires option needs to be set to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought Would be good to have a flag, like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like it, although I don't think we should be doing Date manipulation inside the cookie library so I'm proposing we set it to a fixed value in the future. |
||
secure?: boolean | ||
} | ||
|
||
export function put(key: string, value: string | null, options: CookieOptions = {}): void { | ||
const defaultExpires = 'Thu, 01 Jan 1970 00:00:01 GMT' | ||
let expires = options.expires | ||
if (value == null) expires = 'Thu, 01 Jan 1970 00:00:01 GMT' | ||
if (value == null) expires = defaultExpires | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question Not sure I understand the business requirement here. Comments would be good. A private function encapsulating the business rule would be good, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. I don't think I understand it either 😅 This is copied from the aurelia-plugins-cookies codebase. By just looking at the code, I think there's 2 options: either the cookie is born invalid already or it never expires. Running some tests to see. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I figured it out by testing the implementation directly in the browser. Turns out this defaultExpires is only used for the |
||
if (typeof expires === 'string') expires = new Date(expires) | ||
let str = `${_encode(key)}=${value != null ? _encode(value) : ''}` | ||
if (options.path) str += `; path=${options.path}` | ||
if (options.domain) str += `; domain=${options.domain}` | ||
if (options.expires) str += `; expires=${expires.toUTCString()}` | ||
if (options.expires) str += `; expires=${expires?.toUTCString() ?? defaultExpires}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise Much better. |
||
if (options.secure) str += '; secure' | ||
document.cookie = str | ||
} | ||
|
||
export function putObject(key: string, value: Record<string, unknown>, options = {}): void { | ||
export function putObject(key: string, value: Record<string, unknown>, options: CookieOptions = {}): void { | ||
put(key, JSON.stringify(value), options) | ||
} | ||
|
||
export function remove(key: string, options = {}): void { | ||
export function remove(key: string, options: CookieOptions = {}): void { | ||
put(key, null, options) | ||
} | ||
|
||
|
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.
No need for dateFormatter anymore cause that was used for the
expires
option which is now configurable from the client.