Skip to content
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(cookies): make RequestCookies and ResponseCookies more spec compliant #177

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .changeset/unlucky-icons-heal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
'@edge-runtime/cookies': major
---

Make `RequestCookies` and `ResponseCookies` more spec compliant by resembling the [Cookie Store API](https://wicg.github.io/cookie-store). The main difference is that the methods do not return `Promise`.

Breaking changes:

- `ResponseCookies#get` has been renamed to `ResponseCookies#getValue`
- `ResponseCookies#getWithOptions` has been renamed to `ResponseCookies#get`
2 changes: 1 addition & 1 deletion packages/cookies/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export type { Options } from './serialize'
export type { Cookie, CookieListItem } from './serialize'
Copy link
Member

@Kikobeats Kikobeats Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CookieOptions looks more natural to me, although I can see this is a thing defined by the spec interface

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's essentially the entire parsed cookie, with name and value. Only wording, so I am OK with changing it, but in my mind Cookie made more sense.

export { ResponseCookies } from './response-cookies'
export { RequestCookies } from './request-cookies'
104 changes: 65 additions & 39 deletions packages/cookies/src/request-cookies.ts
Original file line number Diff line number Diff line change
@@ -1,80 +1,106 @@
import { parseCookieString, serialize } from './serialize'
import { type Cookie, parseCookieString, serialize } from './serialize'
import { cached } from './cached'

/**
* A class for manipulating {@link Request} cookies.
*/
export class RequestCookies {
private readonly headers: Headers
readonly #headers: Headers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the future is here and now


constructor(request: Request) {
this.headers = request.headers
this.#headers = request.headers
}

/**
* Delete all the cookies in the cookies in the request
*/
clear(): void {
this.delete([...this.parsed().keys()])
#cache = cached((header: string | null) => {
const parsed = header ? parseCookieString(header) : new Map()
return parsed
})

#parsed(): Map<string, string> {
const header = this.#headers.get('cookie')
return this.#cache(header)
}

/**
* Format the cookies in the request as a string for logging
*/
[Symbol.for('edge-runtime.inspect.custom')]() {
return `RequestCookies ${JSON.stringify(Object.fromEntries(this.parsed()))}`
[Symbol.iterator]() {
return this.#parsed()[Symbol.iterator]()
}

/**
* The amount of cookies received from the client
*/
get size(): number {
return this.parsed().size
return this.#parsed().size
}

[Symbol.iterator]() {
return this.parsed()[Symbol.iterator]()
get(...args: [name: string] | [Cookie]) {
const name = typeof args[0] === 'string' ? args[0] : args[0].name
return this.#parsed().get(name)
}

private cache = cached((header: string | null) => {
const parsed = header ? parseCookieString(header) : new Map()
return parsed
})

private parsed(): Map<string, string> {
const header = this.headers.get('cookie')
return this.cache(header)
}
getAll(...args: [name: string] | [Cookie] | [undefined]) {
const all = Array.from(this.#parsed())
if (!args.length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a strict comparison could be desirable

args.length === 0

Copy link
Member Author

@balazsorban44 balazsorban44 Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of TypeScript, this would show:

This condition will always return 'false' since the types '1' and '0' have no overlap.ts(2367)

Since we technically don't allow more than one argument, but the user might/will try. !args.length should be as safe, since args is always going to be an array.

return all
}

get(name: string) {
return this.parsed().get(name)
const name = typeof args[0] === 'string' ? args[0] : args[0]?.name
return all.filter(([n]) => n === name)
}

has(name: string) {
return this.parsed().has(name)
return this.#parsed().has(name)
}

set(name: string, value: string): this {
const map = this.parsed()
map.set(name, value)
this.headers.set(
set(...args: [key: string, value: string] | [options: Cookie]): this {
const [key, value] =
args.length === 1 ? [args[0].name, args[0].value, args[0]] : args

const map = this.#parsed()
map.set(key, value)

this.#headers.set(
'cookie',
[...map].map(([key, value]) => serialize(key, value, {})).join('; ')
Array.from(map)
.map(([name, value]) => serialize({ name, value }))
.join('; ')
)
return this
}

delete(names: string[]): boolean[]
delete(name: string): boolean
delete(names: string | string[]): boolean | boolean[] {
const map = this.parsed()
/**
* Delete the cookies matching the passed name or names in the request.
*/
delete(
/** Name or names of the cookies to be deleted */
names: string | string[]
): boolean | boolean[] {
const map = this.#parsed()
const result = !Array.isArray(names)
? map.delete(names)
: names.map((name) => map.delete(name))
this.headers.set(
this.#headers.set(
'cookie',
[...map].map(([key, value]) => serialize(key, value, {})).join('; ')
Array.from(map)
.map(([name, value]) => serialize({ name, value }))
.join('; ')
)
return result
}

/**
* Delete all the cookies in the cookies in the request.
*/
clear(): this {
this.delete(Array.from(this.#parsed().keys()))
return this
}

/**
* Format the cookies in the request as a string for logging
*/
[Symbol.for('edge-runtime.inspect.custom')]() {
return `RequestCookies ${JSON.stringify(
Object.fromEntries(this.#parsed())
)}`
}
}
120 changes: 79 additions & 41 deletions packages/cookies/src/response-cookies.ts
Original file line number Diff line number Diff line change
@@ -1,88 +1,126 @@
import { cached } from './cached'
import { type Options, parseSetCookieString, serialize } from './serialize'
import { type Cookie, parseSetCookieString, serialize } from './serialize'

type ParsedCookie = { value: string; options: Options }
export type CookieBag = Map<string, ParsedCookie>
export type CookieBag = Map<string, Cookie>

/**
* Loose implementation of the experimental [Cookie Store API](https://wicg.github.io/cookie-store/#dictdef-cookie)
* The main difference is `ResponseCookies` methods do not return a Promise.
*/
export class ResponseCookies {
private readonly headers: Headers
readonly #headers: Headers

constructor(response: Response) {
this.headers = response.headers
this.#headers = response.headers
}

private cache = cached((_key: string | null) => {
// @ts-ignore
const headers = this.headers.getAll('set-cookie')
const map = new Map<string, ParsedCookie>()
#cache = cached(() => {
// @ts-expect-error See https://github.com/whatwg/fetch/issues/973
const headers = this.#headers.getAll('set-cookie')
const map = new Map<string, Cookie>()

for (const header of headers) {
const parsed = parseSetCookieString(header)
if (parsed) {
map.set(parsed.name, {
value: parsed.value,
options: parsed?.attributes,
})
map.set(parsed.name, parsed)
}
}

return map
})

private parsed() {
const allCookies = this.headers.get('set-cookie')
return this.cache(allCookies)
#parsed() {
const allCookies = this.#headers.get('set-cookie')
return this.#cache(allCookies)
}

set(key: string, value: string, options?: Options): this {
const map = this.parsed()
map.set(key, { value, options: normalizeCookieOptions(options || {}) })
replace(map, this.headers)
/**
* {@link https://wicg.github.io/cookie-store/#CookieStore-get CookieStore#get} without the Promise.
*/
get(...args: [key: string] | [options: Cookie]): Cookie | undefined {
const key = typeof args[0] === 'string' ? args[0] : args[0].name
return this.#parsed().get(key)
}
/**
* {@link https://wicg.github.io/cookie-store/#CookieStore-getAll CookieStore#getAll} without the Promise.
*/
getAll(...args: [key: string] | [options: Cookie] | [undefined]): Cookie[] {
const all = Array.from(this.#parsed().values())
if (!args.length) {
return all
}

const key = typeof args[0] === 'string' ? args[0] : args[0]?.name
return all.filter((c) => c.name === key)
}

/**
* {@link https://wicg.github.io/cookie-store/#CookieStore-set CookieStore#set} without the Promise.
*/
set(
...args:
| [key: string, value: string, cookie?: Partial<Cookie>]
| [options: Cookie]
): this {
const [name, value, cookie] =
args.length === 1 ? [args[0].name, args[0].value, args[0]] : args
const map = this.#parsed()
map.set(name, normalizeCookie({ name, value, ...cookie }))
replace(map, this.#headers)

return this
}

delete(key: string): this {
return this.set(key, '', { expires: new Date(0) })
/**
* {@link https://wicg.github.io/cookie-store/#CookieStore-delete CookieStore#delete} without the Promise.
*/
delete(...args: [key: string] | [options: Cookie]): this {
const name = typeof args[0] === 'string' ? args[0] : args[0].name
return this.set({ name, value: '', expires: new Date(0) })
}

get(key: string): string | undefined {
return this.getWithOptions(key)?.value
// Non-spec

/**
* Uses {@link ResponseCookies.get} to return only the cookie value.
*/
getValue(...args: [key: string] | [options: Cookie]): string | undefined {
return this.get(...args)?.value
Copy link
Member Author

@balazsorban44 balazsorban44 Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably, this method should be unnecessary:
.getValue("cookie") is the same as
.get("cookie")?.value

Breaking spec to save 2 characters might not be worth it.

}

getWithOptions(key: string): {
value: string | undefined
options: Options
} {
const element = this.parsed().get(key)
return { value: element?.value, options: element?.options ?? {} }
/**
* Uses {@link ResponseCookies.delete} to invalidate all cookies matching the given name.
* If no name is provided, all cookies are invalidated.
*/
clear(...args: [key: string] | [options: Cookie] | [undefined]): this {
const key = typeof args[0] === 'string' ? args[0] : args[0]?.name
this.getAll(key).forEach((c) => this.delete(c))
return this
}

[Symbol.for('edge-runtime.inspect.custom')]() {
return `ResponseCookies ${JSON.stringify(
Object.fromEntries(this.parsed())
Object.fromEntries(this.#parsed())
)}`
}
}

function replace(bag: CookieBag, headers: Headers) {
headers.delete('set-cookie')
for (const [key, { value, options }] of bag) {
const serialized = serialize(key, value, options)
for (const [, value] of bag) {
const serialized = serialize(value)
headers.append('set-cookie', serialized)
}
}

const normalizeCookieOptions = (options: Options) => {
options = Object.assign({}, options)

if (options.maxAge) {
options.expires = new Date(Date.now() + options.maxAge * 1000)
function normalizeCookie(cookie: Cookie = { name: '', value: '' }) {
if (cookie.maxAge) {
cookie.expires = new Date(Date.now() + cookie.maxAge * 1000)
}

if (options.path === null || options.path === undefined) {
options.path = '/'
if (cookie.path === null || cookie.path === undefined) {
cookie.path = '/'
}

return options
return cookie
}
Loading