-
Notifications
You must be signed in to change notification settings - Fork 2
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: moving cookie related logic to cookieAdapter.ts #42
Changes from 12 commits
c543587
e2c1ade
bfa3832
ec31aa1
63d1100
474cc58
35092da
47baa47
71bfdbf
6b69641
d8de8c5
2603f50
ff31f67
b0592b1
f4ccc54
b4f36a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,42 +1,33 @@ | ||
import { IncomingMessage, ServerResponse } from 'node:http' | ||
import { AppSession } from '../session' | ||
|
||
export type MaybePromise<T> = T | Promise<T> | ||
|
||
export type Adapter = { | ||
getItem: (params: { | ||
req: IncomingMessage | ||
res: ServerResponse | ||
clientId: string | ||
spaceId: string | ||
userId: string | ||
key: string | ||
}) => MaybePromise<string | undefined> | ||
getSession: GetSession | ||
setSession: SetSession | ||
removeSession: RemoveSession | ||
hasSession: HasSession | ||
} | ||
|
||
setItem: (params: { | ||
req: IncomingMessage | ||
res: ServerResponse | ||
clientId: string | ||
spaceId: string | ||
userId: string | ||
key: string | ||
value: string | ||
}) => MaybePromise<boolean> | ||
type BaseSessionParams = { | ||
req: IncomingMessage | ||
res: ServerResponse | ||
clientId: string | ||
spaceId: string | ||
userId: string | ||
} | ||
|
||
removeItem: (params: { | ||
req: IncomingMessage | ||
res: ServerResponse | ||
clientId: string | ||
spaceId: string | ||
userId: string | ||
key: string | ||
}) => MaybePromise<boolean> | ||
type GetSession = ( | ||
params: BaseSessionParams, | ||
) => MaybePromise<AppSession | undefined> | ||
|
||
hasItem: (params: { | ||
req: IncomingMessage | ||
res: ServerResponse | ||
clientId: string | ||
spaceId: string | ||
userId: string | ||
key: string | ||
}) => MaybePromise<boolean> | ||
} | ||
type SetSession = ( | ||
params: BaseSessionParams & { | ||
session: AppSession | ||
}, | ||
) => MaybePromise<boolean> | ||
|
||
type RemoveSession = (params: BaseSessionParams) => MaybePromise<boolean> | ||
|
||
type HasSession = (params: BaseSessionParams) => MaybePromise<boolean> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ export const changedCookieHeaderValue = ( | |
return [ | ||
`${name}=${value}`, | ||
'path=/', | ||
expires ? `Expires=${expires.toISOString()}; ` : undefined, | ||
expires ? `Expires=${expires}; ` : undefined, | ||
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. @eunjae-lee I changes this because it did not work for me and the expires attribute was 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. You're right. I've just confirmed it! |
||
'samesite=none', | ||
'secure', | ||
'httponly', | ||
|
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.
WDYT about this comment @eunjae-lee ?
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.
if you agree with this I will also need to change a bit the docs (just a note for myself)
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.
Yeah agree with create... naming
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.
on it
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.
Done ff31f67