Skip to content

Commit

Permalink
Avoid void (#331)
Browse files Browse the repository at this point in the history 
* Avoid using void.

* Avoid using no-op callback.

Instead, change `putCookie` invocation depending on whether `callback` is provided.

* Misc cleanup.

* Prefer `promiseCallback.resolve` where possible.

`promiseCallback.resolve()` is implemented as `cb(); return promise`,
so doing `promiseCallback.callback(); return promiseCallback.promise`
is unnecessary.

---------

Co-authored-by: Colin Casey <casey.colin@gmail.com>
  • Loading branch information
@wjhsf @colincasey
wjhsf and colincasey authored Jan 16, 2024
1 parent ed834af commit 1faaf15
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 138 deletions.
2 changes: 0 additions & 2 deletions .eslintrc.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
},
"reportUnusedDisableDirectives": true,
"rules": {
// Causes a lot of errors - will address separately
"@typescript-eslint/no-invalid-void-type": "off",
"max-lines": ["warn", 500]
}
}
63 changes: 26 additions & 37 deletions lib/__tests__/removeAll.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { Cookie } from '../cookie/cookie'
import { CookieJar } from '../cookie/cookieJar'
import { MemoryCookieStore } from '../memstore'
import { Store } from '../store'
import type { Callback } from '../utils'
import type { Callback, ErrorCallback } from '../utils'

const url = 'http://example.com/index.html'

Expand Down Expand Up @@ -33,24 +33,20 @@ describe('store removeAllCookies API', () => {
// replace remove cookie behavior to throw an error on the 4th invocation
const _removeCookie = store.removeCookie.bind(store)
const spy = jest.spyOn(store, 'removeCookie')
spy.mockImplementationOnce(
(domain: string, path: string, key: string, callback: Callback<void>) =>
_removeCookie.call(store, domain, path, key, callback),
spy.mockImplementationOnce((domain, path, key, callback) =>
_removeCookie.call(store, domain, path, key, callback),
)
spy.mockImplementationOnce(
(domain: string, path: string, key: string, callback: Callback<void>) =>
_removeCookie.call(store, domain, path, key, callback),
spy.mockImplementationOnce((domain, path, key, callback) =>
_removeCookie.call(store, domain, path, key, callback),
)
spy.mockImplementationOnce(
(domain: string, path: string, key: string, callback: Callback<void>) =>
_removeCookie.call(store, domain, path, key, callback),
spy.mockImplementationOnce((domain, path, key, callback) =>
_removeCookie.call(store, domain, path, key, callback),
)
spy.mockImplementationOnce(
(_domain, _path, _key, callback: Callback<void>) =>
callback(new Error('something happened 4')),
spy.mockImplementationOnce((_domain, _path, _key, callback) =>
callback(new Error('something happened 4')),
)

await expect(jar.removeAllCookies()).rejects.toThrowError(
await expect(jar.removeAllCookies()).rejects.toThrow(
'something happened 4',
)

Expand All @@ -73,21 +69,14 @@ describe('store removeAllCookies API', () => {
// replace remove cookie behavior to throw an error on the 4th invocation
const _removeCookie = store.removeCookie.bind(store)
const spy = jest.spyOn(store, 'removeCookie')
spy.mockImplementation(
(
domain: string,
path: string,
key: string,
callback: Callback<void>,
) => {
if (spy.mock.calls.length % 2 === 1) {
return callback(
new Error(`something happened ${spy.mock.calls.length}`),
)
}
return _removeCookie.call(store, domain, path, key, callback)
},
)
spy.mockImplementation((domain, path, key, callback) => {
if (spy.mock.calls.length % 2 === 1) {
return callback(
new Error(`something happened ${spy.mock.calls.length}`),
)
}
return _removeCookie.call(store, domain, path, key, callback)
})

await expect(jar.removeAllCookies()).rejects.toThrowError(
'something happened 1',
Expand Down Expand Up @@ -181,8 +170,8 @@ class StoreWithoutRemoveAll extends Store {
}

override putCookie(cookie: Cookie): Promise<void>
override putCookie(cookie: Cookie, callback: Callback<void>): void
override putCookie(cookie: Cookie, callback?: Callback<void>): unknown {
override putCookie(cookie: Cookie, callback: ErrorCallback): void
override putCookie(cookie: Cookie, callback?: ErrorCallback): unknown {
this.stats.put++
this.cookies.push(cookie)
if (!callback) {
Expand Down Expand Up @@ -210,13 +199,13 @@ class StoreWithoutRemoveAll extends Store {
domain: string,
path: string,
key: string,
callback: Callback<void>,
callback: ErrorCallback,
): void
override removeCookie(
_domain: string,
_path: string,
_key: string,
callback?: Callback<void>,
callback?: ErrorCallback,
): unknown {
this.stats.remove++
if (!callback) {
Expand Down Expand Up @@ -257,13 +246,13 @@ class MemoryStoreExtension extends MemoryCookieStore {
domain: string,
path: string,
key: string,
callback: Callback<void>,
callback: ErrorCallback,
): void
override removeCookie(
domain: string,
path: string,
key: string,
callback?: Callback<void>,
callback?: ErrorCallback,
): unknown {
this.stats.remove++
if (!callback) {
Expand All @@ -273,8 +262,8 @@ class MemoryStoreExtension extends MemoryCookieStore {
}

override removeAllCookies(): Promise<void>
override removeAllCookies(callback: Callback<void>): void
override removeAllCookies(callback?: Callback<void>): unknown {
override removeAllCookies(callback: ErrorCallback): void
override removeAllCookies(callback?: ErrorCallback): unknown {
this.stats.removeAll++
if (!callback) {
throw new Error('This should not be undefined')
Expand Down
81 changes: 35 additions & 46 deletions lib/cookie/cookieJar.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { pathMatch } from '../pathMatch'
import { Cookie } from './cookie'
import {
Callback,
ErrorCallback,
createPromiseCallback,
inOperator,
safeToString,
Expand Down Expand Up @@ -169,15 +170,19 @@ export class CookieJar {
this.store = store ?? new MemoryCookieStore()
}

private callSync<T>(fn: (callback: Callback<T>) => void): T | undefined {
private callSync<T>(
// Using tuples is needed to check if `T` is `never` because `T extends never ? true : false`
// evaluates to `never` instead of `true`.
fn: (callback: [T] extends [never] ? ErrorCallback : Callback<T>) => void,
): T | undefined {
if (!this.store.synchronous) {
throw new Error(
'CookieJar store is not synchronous; use async API instead.',
)
}
let syncErr: Error | null = null
let syncResult: T | undefined = undefined
fn.call(this, (error, result) => {
fn.call(this, (error: Error | null, result?: T | undefined) => {
syncErr = error
syncResult = result
})
Expand Down Expand Up @@ -413,22 +418,14 @@ export class CookieJar {
const store = this.store

if (!store.updateCookie) {
store.updateCookie = function (
store.updateCookie = async function (
_oldCookie: Cookie,
newCookie: Cookie,
cb?: Callback<void>,
cb?: ErrorCallback,
): Promise<void> {
return this.putCookie(newCookie).then(
() => {
if (cb) {
cb(null, undefined)
}
},
(error: Error) => {
if (cb) {
cb(error, undefined)
}
},
() => cb?.(null),
(error: Error) => cb?.(error),
)
}
}
Expand Down Expand Up @@ -492,13 +489,10 @@ export class CookieJar {
url: string,
options?: SetCookieOptions,
): Cookie | undefined {
const setCookieFn = this.setCookie.bind(
this,
cookie,
url,
options as SetCookieOptions,
)
return this.callSync<Cookie | undefined>(setCookieFn)
const setCookieFn = options
? this.setCookie.bind(this, cookie, url, options)
: this.setCookie.bind(this, cookie, url)
return this.callSync(setCookieFn)
}

// RFC6365 S5.4
Expand Down Expand Up @@ -659,9 +653,7 @@ export class CookieJar {
return promiseCallback.promise
}
getCookiesSync(url: string, options?: GetCookiesOptions): Cookie[] {
return (
this.callSync<Cookie[]>(this.getCookies.bind(this, url, options)) ?? []
)
return this.callSync(this.getCookies.bind(this, url, options)) ?? []
}

getCookieString(
Expand All @@ -686,19 +678,14 @@ export class CookieJar {
options = undefined
}
const promiseCallback = createPromiseCallback(callback)
const next: Callback<Cookie[]> = function (
err: Error | null,
cookies: Cookie[] | undefined,
) {
const next: Callback<Cookie[]> = function (err, cookies) {
if (err) {
promiseCallback.callback(err)
} else if (cookies === undefined) {
promiseCallback.callback(null, cookies)
} else {
promiseCallback.callback(
null,
cookies
.sort(cookieCompare)
?.sort(cookieCompare)
.map((c) => c.cookieString())
.join('; '),
)
Expand All @@ -710,8 +697,10 @@ export class CookieJar {
}
getCookieStringSync(url: string, options?: GetCookiesOptions): string {
return (
this.callSync<string | undefined>(
this.getCookieString.bind(this, url, options as GetCookiesOptions),
this.callSync(
options
? this.getCookieString.bind(this, url, options)
: this.getCookieString.bind(this, url),
) ?? ''
)
}
Expand Down Expand Up @@ -773,9 +762,7 @@ export class CookieJar {
options: GetCookiesOptions = {},
): string[] {
return (
this.callSync<string[] | undefined>(
this.getSetCookieStrings.bind(this, url, options),
) ?? []
this.callSync(this.getSetCookieStrings.bind(this, url, options)) ?? []
)
}

Expand Down Expand Up @@ -851,9 +838,7 @@ export class CookieJar {
return promiseCallback.promise
}
serializeSync(): SerializedCookieJar | undefined {
return this.callSync<SerializedCookieJar>((callback) => {
this.serialize(callback)
})
return this.callSync((callback) => this.serialize(callback))
}

toJSON() {
Expand Down Expand Up @@ -959,10 +944,10 @@ export class CookieJar {
return this._cloneSync(newStore)
}

removeAllCookies(callback: Callback<void>): void
removeAllCookies(callback: ErrorCallback): void
removeAllCookies(): Promise<void>
removeAllCookies(callback?: Callback<void>): unknown {
const promiseCallback = createPromiseCallback<void>(callback)
removeAllCookies(callback?: ErrorCallback): unknown {
const promiseCallback = createPromiseCallback<undefined>(callback)
const cb = promiseCallback.callback

const store = this.store
Expand All @@ -974,7 +959,9 @@ export class CookieJar {
typeof store.removeAllCookies === 'function' &&
store.removeAllCookies !== Store.prototype.removeAllCookies
) {
void store.removeAllCookies(cb)
// `Callback<undefined>` and `ErrorCallback` are *technically* incompatible, but for the
// standard implementation `cb = (err, result) => {}`, they're essentially the same.
void store.removeAllCookies(cb as ErrorCallback)
return promiseCallback.promise
}

Expand All @@ -989,7 +976,7 @@ export class CookieJar {
}

if (cookies.length === 0) {
cb(null)
cb(null, undefined)
return
}

Expand All @@ -1005,7 +992,7 @@ export class CookieJar {

if (completedCount === cookies?.length) {
if (removeErrors[0]) cb(removeErrors[0])
else cb(null)
else cb(null, undefined)
return
}
}
Expand All @@ -1023,7 +1010,9 @@ export class CookieJar {
return promiseCallback.promise
}
removeAllCookiesSync(): void {
return this.callSync<void>((callback) => this.removeAllCookies(callback))
return this.callSync<never>((callback) => {
return this.removeAllCookies(callback)
})
}

static deserialize(
Expand Down
Loading

0 comments on commit 1faaf15

Please sign in to comment.