From 24025136dcc0e8918bc4d3a242eb2782cfe15e83 Mon Sep 17 00:00:00 2001 From: Will Harney Date: Thu, 26 Oct 2023 15:31:10 -0400 Subject: [PATCH 01/11] Ignore max-lines where appropriate. --- lib/__tests__/data/parser.ts | 2 ++ lib/cookie/cookie.ts | 9 ++++++++- lib/cookie/cookieJar.ts | 3 +++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/__tests__/data/parser.ts b/lib/__tests__/data/parser.ts index 7618a7bf..3c93221a 100644 --- a/lib/__tests__/data/parser.ts +++ b/lib/__tests__/data/parser.ts @@ -1,3 +1,5 @@ +// This file just contains test data, so we don't care about the number of lines. +/* eslint-disable max-lines */ export default [ { test: '0001', diff --git a/lib/cookie/cookie.ts b/lib/cookie/cookie.ts index 15052309..5ad402b2 100644 --- a/lib/cookie/cookie.ts +++ b/lib/cookie/cookie.ts @@ -29,6 +29,9 @@ * POSSIBILITY OF SUCH DAMAGE. */ +// This file was too big before we added max-lines, and it's ongoing work to reduce its size. +/* eslint max-lines: [1, 750] */ + import * as pubsuffix from '../pubsuffix-psl' import * as validators from '../validators' import { getCustomInspectSymbol } from '../utilHelper' @@ -540,7 +543,11 @@ export class Cookie { ) { return false } - if (this.maxAge != null && this.maxAge <= 0) { + if ( + this.maxAge != null && + this.maxAge !== 'Infinity' && + (this.maxAge === '-Infinity' || this.maxAge <= 0) + ) { return false // "Max-Age=" non-zero-digit *DIGIT } if (this.path != null && !PATH_VALUE.test(this.path)) { diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts index aff9c2be..cdc550fe 100644 --- a/lib/cookie/cookieJar.ts +++ b/lib/cookie/cookieJar.ts @@ -24,6 +24,9 @@ import { domainMatch } from './domainMatch' import { cookieCompare } from './cookieCompare' import { version } from '../version' +// This file was too big before we added max-lines, and it's ongoing work to reduce its size. +/* eslint max-lines: [1, 1200] */ + const defaultSetCookieOptions: SetCookieOptions = { loose: false, sameSiteContext: undefined, From 421b195893b84962e913b56073186a9733494728 Mon Sep 17 00:00:00 2001 From: Will Harney Date: Thu, 26 Oct 2023 15:48:34 -0400 Subject: [PATCH 02/11] Change callback error signature to null instead of undefined. --- lib/__tests__/removeAll.spec.ts | 10 +++++----- lib/cookie/cookieJar.ts | 32 ++++++++++++++++---------------- lib/utils.ts | 9 +++------ 3 files changed, 24 insertions(+), 27 deletions(-) diff --git a/lib/__tests__/removeAll.spec.ts b/lib/__tests__/removeAll.spec.ts index 1237df7b..c7b3602a 100644 --- a/lib/__tests__/removeAll.spec.ts +++ b/lib/__tests__/removeAll.spec.ts @@ -154,7 +154,7 @@ class StoreWithoutRemoveAll extends Store { if (!callback) { throw new Error('This should not be undefined') } - return callback(undefined, undefined) + return callback(null, undefined) } override findCookies( @@ -177,7 +177,7 @@ class StoreWithoutRemoveAll extends Store { if (!callback) { throw new Error('This should not be undefined') } - return callback(undefined, []) + return callback(null, []) } override putCookie(cookie: Cookie): Promise @@ -188,7 +188,7 @@ class StoreWithoutRemoveAll extends Store { if (!callback) { throw new Error('This should not be undefined') } - return callback(undefined) + return callback(null) } override getAllCookies(): Promise @@ -198,7 +198,7 @@ class StoreWithoutRemoveAll extends Store { if (!callback) { throw new Error('This should not be undefined') } - return callback(undefined, this.cookies.slice()) + return callback(null, this.cookies.slice()) } override removeCookie( @@ -222,7 +222,7 @@ class StoreWithoutRemoveAll extends Store { if (!callback) { throw new Error('This should not be undefined') } - return callback(undefined) + return callback(null) } } diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts index cdc550fe..ce94dcb4 100644 --- a/lib/cookie/cookieJar.ts +++ b/lib/cookie/cookieJar.ts @@ -176,7 +176,7 @@ export class CookieJar { 'CookieJar store is not synchronous; use async API instead.', ) } - let syncErr: Error | undefined + let syncErr: Error | null = null let syncResult: T | undefined = undefined fn.call(this, (error, result) => { syncErr = error @@ -406,7 +406,7 @@ export class CookieJar { return this.putCookie(newCookie).then( () => { if (cb) { - cb(undefined, undefined) + cb(null, undefined) } }, (error: Error) => { @@ -419,7 +419,7 @@ export class CookieJar { } function withCookie( - err: Error | undefined, + err: Error | null, oldCookie: Cookie | undefined | null, ): void { if (err) { @@ -427,7 +427,7 @@ export class CookieJar { return } - const next = function (err: Error | undefined): void { + const next = function (err: Error | null): void { if (err || typeof cookie === 'string') { cb(err) } else { @@ -616,7 +616,7 @@ export class CookieJar { } if (cookies == null) { - cb(undefined, []) + cb(null, []) return } @@ -672,14 +672,14 @@ export class CookieJar { } const next: Callback = function ( - err: Error | undefined, + err: Error | null, cookies: Cookie[] | undefined, ) { if (err || cookies === undefined) { promiseCallback.callback(err) } else { promiseCallback.callback( - undefined, + null, cookies .sort(cookieCompare) .map((c) => c.cookieString()) @@ -728,7 +728,7 @@ export class CookieJar { } const next: Callback = function ( - err: Error | undefined, + err: Error | null, cookies: Cookie[] | undefined, ) { if (err || cookies === undefined) { @@ -812,7 +812,7 @@ export class CookieJar { } if (cookies == null) { - promiseCallback.callback(undefined, serialized) + promiseCallback.callback(null, serialized) return } @@ -826,7 +826,7 @@ export class CookieJar { return serializedCookie }) - promiseCallback.callback(undefined, serialized) + promiseCallback.callback(null, serialized) }) return promiseCallback.promise @@ -863,7 +863,7 @@ export class CookieJar { cookies = cookies.slice() // do not modify the original - const putNext = (err?: Error): void => { + const putNext = (err: Error | null): void => { if (err) { return callback(err, undefined) } @@ -881,14 +881,14 @@ export class CookieJar { } if (cookie === null) { - return putNext(undefined) // skip this cookie + return putNext(null) // skip this cookie } this.store.putCookie(cookie, putNext) } } - putNext() + putNext(null) } _importCookiesSync(serialized: unknown): void { @@ -980,7 +980,7 @@ export class CookieJar { let completedCount = 0 const removeErrors: Error[] = [] - function removeCookieCb(removeErr: Error | undefined) { + function removeCookieCb(removeErr: Error | null) { if (removeErr) { removeErrors.push(removeErr) } @@ -988,7 +988,7 @@ export class CookieJar { completedCount++ if (completedCount === cookies?.length) { - cb(removeErrors.length ? removeErrors[0] : null) + cb(removeErrors[0] ?? null) return } } @@ -1083,7 +1083,7 @@ export class CookieJar { promiseCallback.callback(err) return } - promiseCallback.callback(undefined, jar) + promiseCallback.callback(null, jar) }) return promiseCallback.promise diff --git a/lib/utils.ts b/lib/utils.ts index 8834b140..79ed9f77 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -1,8 +1,5 @@ /** A callback function that accepts an error or a result. */ -export type Callback = ( - error: Error | undefined, - result: T | undefined, -) => void +export type Callback = (error: Error | null, result: T | undefined) => void /** Signature for a callback function that expects an error to be passed. */ export type ErrorCallback = (error: Error, result?: never) => void @@ -25,9 +22,9 @@ export const safeToString = (val: unknown) => { /** Utility object for promise/callback interop. */ export interface PromiseCallback { promise: Promise - callback: (error: Error | undefined | null, result?: T) => void + callback: (error: Error | null, result?: T) => void resolve: (value: T | undefined) => Promise - reject: (error: Error | undefined | null) => Promise + reject: (error: Error | null) => Promise } /** Converts a callback into a utility object where either a callback or a promise can be used. */ From f2df0bb4ddd60d7196ac56d6194663fea71e0f4f Mon Sep 17 00:00:00 2001 From: Will Harney Date: Thu, 26 Oct 2023 15:52:23 -0400 Subject: [PATCH 03/11] Remove unnecessary helper. --- lib/utils.ts | 6 +----- lib/validators.ts | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/utils.ts b/lib/utils.ts index 79ed9f77..4be8b32d 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -4,10 +4,6 @@ export type Callback = (error: Error | null, result: T | undefined) => void /** Signature for a callback function that expects an error to be passed. */ export type ErrorCallback = (error: Error, result?: never) => void -/** Wrapped `Object.prototype.toString`, so that you don't need to remember to use `.call()`. */ -export const objectToString = (obj: unknown) => - Object.prototype.toString.call(obj) - /** Safely converts any value to string, using the value's own `toString` when available. */ export const safeToString = (val: unknown) => { // Ideally, we'd just use String() for everything, but it breaks if `toString` is missing (mostly @@ -15,7 +11,7 @@ export const safeToString = (val: unknown) => { if (val === undefined || val === null || typeof val.toString === 'function') { return String(val) } else { - return objectToString(val) + return Object.prototype.toString.call(val) } } diff --git a/lib/validators.ts b/lib/validators.ts index 1a71adda..cfc4d500 100644 --- a/lib/validators.ts +++ b/lib/validators.ts @@ -26,7 +26,7 @@ SOFTWARE. ************************************************************************************ */ -import { ErrorCallback, objectToString, safeToString } from './utils' +import { ErrorCallback, safeToString } from './utils' /* Validation functions copied from check-types package - https://www.npmjs.com/package/check-types */ @@ -52,7 +52,7 @@ export function isString(data: unknown): boolean { /** Determines whether the string representation of the argument is "[object Object]". */ export function isObject(data: unknown): boolean { - return objectToString(data) === '[object Object]' + return safeToString(data) === '[object Object]' } /** Determines whether the argument is an integer. */ From ca0ec2663e571072b0ae52910f3462221745dad8 Mon Sep 17 00:00:00 2001 From: Will Harney Date: Thu, 26 Oct 2023 16:06:29 -0400 Subject: [PATCH 04/11] Split error callback and success callback for better type narrowing. --- lib/__tests__/removeAll.spec.ts | 6 +++--- lib/utils.ts | 11 +++++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/__tests__/removeAll.spec.ts b/lib/__tests__/removeAll.spec.ts index c7b3602a..3c0db89a 100644 --- a/lib/__tests__/removeAll.spec.ts +++ b/lib/__tests__/removeAll.spec.ts @@ -138,18 +138,18 @@ class StoreWithoutRemoveAll extends Store { domain: string, path: string, key: string, - ): Promise + ): Promise override findCookie( domain: string, path: string, key: string, - callback: Callback, + callback: Callback, ): void override findCookie( _domain: string, _path: string, _key: string, - callback?: Callback, + callback?: Callback, ): unknown { if (!callback) { throw new Error('This should not be undefined') diff --git a/lib/utils.ts b/lib/utils.ts index 4be8b32d..127574b4 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -1,8 +1,11 @@ -/** A callback function that accepts an error or a result. */ -export type Callback = (error: Error | null, result: T | undefined) => void +/** A callback function that expects an error to be passed. */ +export type ErrorCallback = (error: Err, result?: never) => void + +/** A callback function that expects a successful result. */ +type SuccessCallback = (error: null, result: T) => void -/** Signature for a callback function that expects an error to be passed. */ -export type ErrorCallback = (error: Error, result?: never) => void +/** A callback function that accepts an error or a result. */ +export type Callback = SuccessCallback & ErrorCallback /** Safely converts any value to string, using the value's own `toString` when available. */ export const safeToString = (val: unknown) => { From d1b7446bd5a57ca524c234c40e6010e1d79124a4 Mon Sep 17 00:00:00 2001 From: Will Harney Date: Thu, 26 Oct 2023 22:08:00 -0400 Subject: [PATCH 05/11] Be more strict about resolving/rejecting values. --- lib/cookie/cookieJar.ts | 81 ++++++++++++++++++++++++++--------------- lib/utils.ts | 31 +++++++++------- 2 files changed, 68 insertions(+), 44 deletions(-) diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts index ce94dcb4..52e56c81 100644 --- a/lib/cookie/cookieJar.ts +++ b/lib/cookie/cookieJar.ts @@ -218,7 +218,7 @@ export class CookieJar { options?: SetCookieOptions | Callback, callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(arguments) const cb = promiseCallback.callback validators.validate( @@ -245,7 +245,7 @@ export class CookieJar { cookie instanceof String && cookie.length == 0 ) { - return promiseCallback.reject(null) + return promiseCallback.resolve(undefined) } const host = canonicalDomain(context.hostname) @@ -264,7 +264,9 @@ export class CookieJar { const parsedCookie = Cookie.parse(cookie.toString(), { loose: loose }) if (!parsedCookie) { err = new Error('Cookie failed to parse') - return promiseCallback.reject(options?.ignoreError ? null : err) + return options?.ignoreError + ? promiseCallback.resolve(undefined) + : promiseCallback.reject(err) } cookie = parsedCookie } else if (!(cookie instanceof Cookie)) { @@ -273,7 +275,10 @@ export class CookieJar { err = new Error( 'First argument to setCookie must be a Cookie object or string', ) - return promiseCallback.reject(options?.ignoreError ? null : err) + + return options?.ignoreError + ? promiseCallback.resolve(undefined) + : promiseCallback.reject(err) } // S5.3 step 2 @@ -297,18 +302,20 @@ export class CookieJar { if (suffix == null && !IP_V6_REGEX_OBJECT.test(cookie.domain)) { // e.g. "com" err = new Error('Cookie has domain set to a public suffix') - return promiseCallback.reject(options?.ignoreError ? null : err) - } - } catch (err) { - if (options?.ignoreError) { - return promiseCallback.reject(null) - } else { - if (err instanceof Error) { - return promiseCallback.reject(err) - } else { - return promiseCallback.reject(null) - } + + return options?.ignoreError + ? promiseCallback.resolve(undefined) + : promiseCallback.reject(err) } + // Using `any` here rather than `unknown` to avoid a type assertion, at the cost of needing + // to disable eslint directives. It's easier to have this one spot of technically incorrect + // types, rather than having to deal with _all_ callback errors being `unknown`. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } catch (err: any) { + return options?.ignoreError + ? promiseCallback.resolve(undefined) + : // eslint-disable-next-line @typescript-eslint/no-unsafe-argument + promiseCallback.reject(err) } } @@ -322,7 +329,9 @@ export class CookieJar { cookie.cdomain() ?? 'null' } Request:${host ?? 'null'}`, ) - return promiseCallback.reject(options?.ignoreError ? null : err) + return options?.ignoreError + ? promiseCallback.resolve(undefined) + : promiseCallback.reject(err) } if (cookie.hostOnly == null) { @@ -348,7 +357,9 @@ export class CookieJar { // S5.3 step 10 if (options?.http === false && cookie.httpOnly) { err = new Error("Cookie is HttpOnly and this isn't an HTTP API") - return promiseCallback.reject(options?.ignoreError ? null : err) + return options?.ignoreError + ? promiseCallback.resolve(undefined) + : promiseCallback.reject(err) } // 6252bis-02 S5.4 Step 13 & 14: @@ -363,7 +374,9 @@ export class CookieJar { // abort these steps and ignore the newly created cookie entirely." if (sameSiteContext === 'none') { err = new Error('Cookie is SameSite but this is a cross-origin request') - return promiseCallback.reject(options?.ignoreError ? null : err) + return options?.ignoreError + ? promiseCallback.resolve(undefined) + : promiseCallback.reject(err) } } @@ -387,11 +400,9 @@ export class CookieJar { "Cookie has __Host prefix but either Secure or HostOnly attribute is not set or Path is not '/'" } if (errorFound) { - return promiseCallback.reject( - options?.ignoreError || ignoreErrorForPrefixSecurity - ? null - : new Error(errorMsg), - ) + return options?.ignoreError || ignoreErrorForPrefixSecurity + ? promiseCallback.resolve(undefined) + : promiseCallback.reject(new Error(errorMsg)) } } @@ -428,8 +439,10 @@ export class CookieJar { } const next = function (err: Error | null): void { - if (err || typeof cookie === 'string') { + if (err) { cb(err) + } else if (typeof cookie === 'string') { + cb(null, undefined) } else { cb(null, cookie) } @@ -446,7 +459,8 @@ export class CookieJar { ) { // step 11.2 err = new Error("old Cookie is HttpOnly and this isn't an HTTP API") - cb(options.ignoreError ? null : err) + if (options.ignoreError) cb(null, undefined) + else cb(err) return } if (cookie instanceof Cookie) { @@ -665,7 +679,7 @@ export class CookieJar { // eslint-disable-next-line @typescript-eslint/no-unused-vars _callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(arguments) if (typeof options === 'function') { options = undefined @@ -675,8 +689,10 @@ export class CookieJar { err: Error | null, cookies: Cookie[] | undefined, ) { - if (err || cookies === undefined) { + if (err) { promiseCallback.callback(err) + } else if (cookies === undefined) { + promiseCallback.callback(null, undefined) } else { promiseCallback.callback( null, @@ -721,7 +737,9 @@ export class CookieJar { // eslint-disable-next-line @typescript-eslint/no-unused-vars _callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback( + arguments, + ) if (typeof options === 'function') { options = undefined @@ -731,8 +749,10 @@ export class CookieJar { err: Error | null, cookies: Cookie[] | undefined, ) { - if (err || cookies === undefined) { + if (err) { promiseCallback.callback(err) + } else if (cookies === undefined) { + promiseCallback.callback(null, undefined) } else { promiseCallback.callback( null, @@ -988,7 +1008,8 @@ export class CookieJar { completedCount++ if (completedCount === cookies?.length) { - cb(removeErrors[0] ?? null) + if (removeErrors[0]) cb(removeErrors[0]) + else cb(null) return } } diff --git a/lib/utils.ts b/lib/utils.ts index 127574b4..311bc83a 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -1,11 +1,11 @@ /** A callback function that expects an error to be passed. */ -export type ErrorCallback = (error: Err, result?: never) => void +export type ErrorCallback = (error: Error, result?: never) => void /** A callback function that expects a successful result. */ type SuccessCallback = (error: null, result: T) => void /** A callback function that accepts an error or a result. */ -export type Callback = SuccessCallback & ErrorCallback +export type Callback = SuccessCallback & ErrorCallback /** Safely converts any value to string, using the value's own `toString` when available. */ export const safeToString = (val: unknown) => { @@ -20,19 +20,19 @@ export const safeToString = (val: unknown) => { /** Utility object for promise/callback interop. */ export interface PromiseCallback { - promise: Promise - callback: (error: Error | null, result?: T) => void - resolve: (value: T | undefined) => Promise - reject: (error: Error | null) => Promise + promise: Promise + callback: Callback + resolve: (value: T) => Promise + reject: (error: Error) => Promise } /** Converts a callback into a utility object where either a callback or a promise can be used. */ export function createPromiseCallback(args: IArguments): PromiseCallback { - let callback: (error: Error | null | undefined, result: T | undefined) => void - let resolve: (result: T | undefined) => void - let reject: (error: Error | null) => void + let callback: Callback + let resolve: (result: T) => void + let reject: (error: Error) => void - const promise = new Promise((_resolve, _reject) => { + const promise = new Promise((_resolve, _reject) => { resolve = _resolve reject = _reject }) @@ -49,7 +49,10 @@ export function createPromiseCallback(args: IArguments): PromiseCallback { } else { callback = (err, result) => { try { - err ? reject(err) : resolve(result) + // If `err` is null, we know `result` must be `T` + // The assertion isn't *strictly* correct, as `T` could be nullish, but, ehh, good enough... + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + err ? reject(err) : resolve(result!) } catch (e) { reject(e instanceof Error ? e : new Error()) } @@ -59,12 +62,12 @@ export function createPromiseCallback(args: IArguments): PromiseCallback { return { promise, callback, - resolve: (value: T | undefined) => { + resolve: (value: T) => { callback(null, value) return promise }, - reject: (error: Error | null | undefined) => { - callback(error, undefined) + reject: (error: Error) => { + callback(error) return promise }, } From 1df12e989b0a18043769942d7ba0fdb9b79aa79c Mon Sep 17 00:00:00 2001 From: Will Harney Date: Thu, 26 Oct 2023 22:32:42 -0400 Subject: [PATCH 06/11] Change `promiseCallback` to only accept a callback, not `arguments`. --- .eslintrc.json | 4 -- lib/__tests__/cookieJar.spec.ts | 18 +++---- lib/cookie/cookieJar.ts | 92 ++++++++++++++++----------------- lib/memstore.ts | 35 +++++-------- lib/utils.ts | 9 ++-- 5 files changed, 73 insertions(+), 85 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 505d7ad0..6b82019f 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -13,10 +13,6 @@ }, "reportUnusedDisableDirectives": true, "rules": { - // this is only needed because `createPromiseCallback` uses arguments to support the callback-style api - // we should strongly consider dropping the callback and sync api variants (in say v6) to reduce the - // surface area and complexity of tough-cookie - "prefer-rest-params": "warn", "max-lines": ["warn", 500] } } diff --git a/lib/__tests__/cookieJar.spec.ts b/lib/__tests__/cookieJar.spec.ts index 6999c86c..310214eb 100644 --- a/lib/__tests__/cookieJar.spec.ts +++ b/lib/__tests__/cookieJar.spec.ts @@ -49,7 +49,7 @@ describe('CookieJar', () => { }) describe('setCookie', () => { - let cookie: Cookie + let cookie: Cookie | undefined apiVariants( 'should resolve to a Cookie', @@ -83,8 +83,8 @@ describe('CookieJar', () => { }, () => { expect(cookie).toBeInstanceOf(Cookie) - expect(cookie.key).toBe('foo') - expect(cookie.value).toBe('bar') + expect(cookie?.key).toBe('foo') + expect(cookie?.value).toBe('bar') }, ) @@ -185,8 +185,8 @@ describe('CookieJar', () => { lastAccessed: t1, }), ) - expect(cookie.TTL()).toBe(Infinity) - expect(cookie.isPersistent()).toBe(false) + expect(cookie?.TTL()).toBe(Infinity) + expect(cookie?.isPersistent()).toBe(false) // updates the last access when retrieving a cookie jest.advanceTimersByTime(10000) @@ -279,7 +279,7 @@ describe('CookieJar', () => { 'a=b; Domain=example.com; Path=/', 'http://www.app.example.com/index.html', ) - expect(cookie.domain).toBe('example.com') + expect(cookie?.domain).toBe('example.com') }) it('should allow a sub-path cookie on a super-domain', async () => { @@ -348,8 +348,8 @@ describe('CookieJar', () => { lastAccessed: t0, }), ) - expect(cookie.TTL()).toBe(Infinity) - expect(cookie.isPersistent()).toBe(false) + expect(cookie?.TTL()).toBe(Infinity) + expect(cookie?.isPersistent()).toBe(false) }) }) @@ -800,7 +800,7 @@ describe('CookieJar', () => { }) describe('getSetCookieStrings', () => { - let cookieHeaders: string[] + let cookieHeaders: string[] | undefined describe('api', () => { beforeEach(async () => { diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts index 52e56c81..9a3164a6 100644 --- a/lib/cookie/cookieJar.ts +++ b/lib/cookie/cookieJar.ts @@ -8,7 +8,6 @@ import { pathMatch } from '../pathMatch' import { Cookie } from './cookie' import { Callback, - ErrorCallback, createPromiseCallback, inOperator, safeToString, @@ -189,36 +188,38 @@ export class CookieJar { return syncResult } + // TODO: We *could* add overloads based on the value of `options.ignoreError`, such that we only + // return `undefined` when `ignoreError` is true. But would that be excessive overloading? setCookie( cookie: string | Cookie, url: string, - callback: Callback, + callback: Callback, ): void setCookie( cookie: string | Cookie, url: string, options: SetCookieOptions, - callback: Callback, + callback: Callback, ): void - setCookie(cookie: string | Cookie, url: string): Promise + setCookie(cookie: string | Cookie, url: string): Promise setCookie( cookie: string | Cookie, url: string, options: SetCookieOptions, - ): Promise + ): Promise setCookie( cookie: string | Cookie, url: string, - options: SetCookieOptions | Callback, - callback?: Callback, + options: SetCookieOptions | Callback, + callback?: Callback, ): unknown setCookie( cookie: string | Cookie, url: string, options?: SetCookieOptions | Callback, - callback?: Callback, + callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback validators.validate( @@ -494,7 +495,7 @@ export class CookieJar { url, options as SetCookieOptions, ) - return this.callSync(setCookieFn) + return this.callSync(setCookieFn) } // RFC6365 S5.4 @@ -517,10 +518,9 @@ export class CookieJar { getCookies( url: string, options?: GetCookiesOptions | Callback, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback validators.validate(validators.isNonEmptyString(url), cb, url) @@ -663,23 +663,22 @@ export class CookieJar { getCookieString( url: string, options: GetCookiesOptions, - callback: Callback, + callback: Callback, ): void - getCookieString(url: string, callback: Callback): void + getCookieString(url: string, callback: Callback): void getCookieString(url: string): Promise getCookieString(url: string, options: GetCookiesOptions): Promise getCookieString( url: string, - options: GetCookiesOptions | Callback, - callback?: Callback, + options: GetCookiesOptions | Callback, + callback?: Callback, ): unknown getCookieString( url: string, - options?: GetCookiesOptions | Callback, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + options?: GetCookiesOptions | Callback, + callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) if (typeof options === 'function') { options = undefined @@ -709,36 +708,38 @@ export class CookieJar { } getCookieStringSync(url: string, options?: GetCookiesOptions): string { return ( - this.callSync( + this.callSync( this.getCookieString.bind(this, url, options as GetCookiesOptions), ) ?? '' ) } - getSetCookieStrings(url: string, callback: Callback): void + getSetCookieStrings( + url: string, + callback: Callback, + ): void getSetCookieStrings( url: string, options: GetCookiesOptions, - callback: Callback, + callback: Callback, ): void - getSetCookieStrings(url: string): Promise + getSetCookieStrings(url: string): Promise getSetCookieStrings( url: string, options: GetCookiesOptions, - ): Promise + ): Promise getSetCookieStrings( url: string, options: GetCookiesOptions, - callback?: Callback, + callback?: Callback, ): unknown getSetCookieStrings( url: string, - options?: GetCookiesOptions | Callback, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + options?: GetCookiesOptions | Callback, + callback?: Callback, ): unknown { const promiseCallback = createPromiseCallback( - arguments, + callback, ) if (typeof options === 'function') { @@ -771,7 +772,7 @@ export class CookieJar { options: GetCookiesOptions = {}, ): string[] { return ( - this.callSync( + this.callSync( this.getSetCookieStrings.bind(this, url, options), ) ?? [] ) @@ -780,10 +781,8 @@ export class CookieJar { serialize(callback: Callback): void serialize(): Promise serialize(callback?: Callback): unknown - // eslint-disable-next-line @typescript-eslint/no-unused-vars - serialize(_callback?: Callback): unknown { - const promiseCallback = - createPromiseCallback(arguments) + serialize(callback?: Callback): unknown { + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback validators.validate(typeof cb === 'function', cb) @@ -921,14 +920,13 @@ export class CookieJar { clone(newStore: Store): Promise clone( newStore?: Store | Callback, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + callback?: Callback, ): unknown { if (typeof newStore === 'function') { newStore = undefined } - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback this.serialize((err, serialized) => { @@ -961,12 +959,11 @@ export class CookieJar { return this._cloneSync(newStore) } - removeAllCookies(callback: ErrorCallback): void + removeAllCookies(callback: Callback): void removeAllCookies(): Promise - removeAllCookies(callback?: ErrorCallback): unknown - // eslint-disable-next-line @typescript-eslint/no-unused-vars - removeAllCookies(_callback?: ErrorCallback): unknown { - const promiseCallback = createPromiseCallback(arguments) + removeAllCookies(callback?: Callback): unknown + removeAllCookies(callback?: Callback): unknown { + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback const store = this.store @@ -978,7 +975,7 @@ export class CookieJar { typeof store.removeAllCookies === 'function' && store.removeAllCookies !== Store.prototype.removeAllCookies ) { - store.removeAllCookies(cb) + void store.removeAllCookies(cb) return promiseCallback.promise } @@ -1052,14 +1049,13 @@ export class CookieJar { static deserialize( strOrObj: string | object, store?: Store | Callback, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + callback?: Callback, ): unknown { if (typeof store === 'function') { store = undefined } - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) let serialized: unknown if (typeof strOrObj === 'string') { diff --git a/lib/memstore.ts b/lib/memstore.ts index 2327c7ef..97305269 100644 --- a/lib/memstore.ts +++ b/lib/memstore.ts @@ -83,10 +83,9 @@ export class MemoryCookieStore extends Store { domain: string | null, path: string | null, key: string | undefined, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback if (domain == null || path == null) { @@ -126,15 +125,14 @@ export class MemoryCookieStore extends Store { domain: string, path: string, allowSpecialUseDomain: boolean | Callback = false, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + callback?: Callback, ): unknown { if (typeof allowSpecialUseDomain === 'function') { allowSpecialUseDomain = true } const results: Cookie[] = [] - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback if (!domain) { @@ -191,9 +189,8 @@ export class MemoryCookieStore extends Store { override putCookie(cookie: Cookie): Promise override putCookie(cookie: Cookie, callback: Callback): void - // eslint-disable-next-line @typescript-eslint/no-unused-vars - override putCookie(cookie: Cookie, _callback?: Callback): unknown { - const promiseCallback = createPromiseCallback(arguments) + override putCookie(cookie: Cookie, callback?: Callback): unknown { + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback const { domain, path, key } = cookie @@ -257,10 +254,9 @@ export class MemoryCookieStore extends Store { domain: string, path: string, key: string, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback const domainEntry = this.idx[domain] @@ -287,10 +283,9 @@ export class MemoryCookieStore extends Store { override removeCookies( domain: string, path: string, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _callback?: Callback, + callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(arguments) + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback const domainEntry = this.idx[domain] @@ -308,9 +303,8 @@ export class MemoryCookieStore extends Store { override removeAllCookies(): Promise override removeAllCookies(callback: Callback): void - // eslint-disable-next-line @typescript-eslint/no-unused-vars - override removeAllCookies(_callback?: Callback): unknown { - const promiseCallback = createPromiseCallback(arguments) + override removeAllCookies(callback?: Callback): unknown { + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback this.idx = Object.create(null) as MemoryCookieStoreIndex @@ -321,9 +315,8 @@ export class MemoryCookieStore extends Store { override getAllCookies(): Promise override getAllCookies(callback: Callback): void - // eslint-disable-next-line @typescript-eslint/no-unused-vars - override getAllCookies(_callback?: Callback): unknown { - const promiseCallback = createPromiseCallback(arguments) + override getAllCookies(callback?: Callback): unknown { + const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback const cookies: Cookie[] = [] diff --git a/lib/utils.ts b/lib/utils.ts index 311bc83a..297576ef 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -27,7 +27,7 @@ export interface PromiseCallback { } /** Converts a callback into a utility object where either a callback or a promise can be used. */ -export function createPromiseCallback(args: IArguments): PromiseCallback { +export function createPromiseCallback(cb?: Callback): PromiseCallback { let callback: Callback let resolve: (result: T) => void let reject: (error: Error) => void @@ -37,11 +37,14 @@ export function createPromiseCallback(args: IArguments): PromiseCallback { reject = _reject }) - const cb: unknown = args[args.length - 1] if (typeof cb === 'function') { callback = (err, result) => { try { - cb(err, result) + if (err) cb(err) + // If `err` is null, we know `result` must be `T` + // The assertion isn't *strictly* correct, as `T` could be nullish, but, ehh, good enough... + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + else cb(null, result!) } catch (e) { reject(e instanceof Error ? e : new Error()) } From c216409524c7d14b8639e2563a3217d13dcaaee8 Mon Sep 17 00:00:00 2001 From: Will Harney Date: Fri, 27 Oct 2023 15:44:08 -0400 Subject: [PATCH 07/11] Hide annoying warnings that pop up in IDE. --- tsconfig.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tsconfig.json b/tsconfig.json index d79eb3f4..8e809bac 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -26,6 +26,9 @@ /* Compatibility */ "ignoreDeprecations": "5.0" }, + "include": [ + "lib" + ], "exclude": [ "jest.config.ts" ] From 0fec493d3034bfec12b4877e2723b8e355274412 Mon Sep 17 00:00:00 2001 From: Will Harney Date: Fri, 27 Oct 2023 15:47:15 -0400 Subject: [PATCH 08/11] Handle different function signatures for callback argument. --- lib/cookie/cookieJar.ts | 36 +++++++++++++++++++++--------------- lib/memstore.ts | 3 +++ 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts index 9a3164a6..a412931d 100644 --- a/lib/cookie/cookieJar.ts +++ b/lib/cookie/cookieJar.ts @@ -216,9 +216,13 @@ export class CookieJar { setCookie( cookie: string | Cookie, url: string, - options?: SetCookieOptions | Callback, + options?: SetCookieOptions | Callback, callback?: Callback, ): unknown { + if (typeof options === 'function') { + callback = options + options = undefined + } const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback @@ -520,16 +524,19 @@ export class CookieJar { options?: GetCookiesOptions | Callback, callback?: Callback, ): unknown { + if (typeof options === 'function') { + callback = options + options = defaultGetCookieOptions + } else if (options === undefined) { + options = defaultGetCookieOptions + } const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback validators.validate(validators.isNonEmptyString(url), cb, url) - const context = getCookieContext(url) - if (typeof options === 'function' || options === undefined) { - options = defaultGetCookieOptions - } validators.validate(validators.isObject(options), cb, safeToString(options)) validators.validate(typeof cb === 'function', cb) + const context = getCookieContext(url) const host = canonicalDomain(context.hostname) const path = context.pathname || '/' @@ -678,12 +685,11 @@ export class CookieJar { options?: GetCookiesOptions | Callback, callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback(callback) - if (typeof options === 'function') { + callback = options options = undefined } - + const promiseCallback = createPromiseCallback(callback) const next: Callback = function ( err: Error | null, cookies: Cookie[] | undefined, @@ -691,7 +697,7 @@ export class CookieJar { if (err) { promiseCallback.callback(err) } else if (cookies === undefined) { - promiseCallback.callback(null, undefined) + promiseCallback.callback(null, cookies) } else { promiseCallback.callback( null, @@ -738,13 +744,13 @@ export class CookieJar { options?: GetCookiesOptions | Callback, callback?: Callback, ): unknown { - const promiseCallback = createPromiseCallback( - callback, - ) - if (typeof options === 'function') { + callback = options options = undefined } + const promiseCallback = createPromiseCallback( + callback, + ) const next: Callback = function ( err: Error | null, @@ -780,7 +786,6 @@ export class CookieJar { serialize(callback: Callback): void serialize(): Promise - serialize(callback?: Callback): unknown serialize(callback?: Callback): unknown { const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback @@ -923,6 +928,7 @@ export class CookieJar { callback?: Callback, ): unknown { if (typeof newStore === 'function') { + callback = newStore newStore = undefined } @@ -961,7 +967,6 @@ export class CookieJar { removeAllCookies(callback: Callback): void removeAllCookies(): Promise - removeAllCookies(callback?: Callback): unknown removeAllCookies(callback?: Callback): unknown { const promiseCallback = createPromiseCallback(callback) const cb = promiseCallback.callback @@ -1052,6 +1057,7 @@ export class CookieJar { callback?: Callback, ): unknown { if (typeof store === 'function') { + callback = store store = undefined } diff --git a/lib/memstore.ts b/lib/memstore.ts index 97305269..441b92df 100644 --- a/lib/memstore.ts +++ b/lib/memstore.ts @@ -128,6 +128,9 @@ export class MemoryCookieStore extends Store { callback?: Callback, ): unknown { if (typeof allowSpecialUseDomain === 'function') { + callback = allowSpecialUseDomain + // TODO: It's weird that `allowSpecialUseDomain` defaults to false with no callback, + // but true with a callback. This is legacy behavior from v4. allowSpecialUseDomain = true } From a7a84f311ab4942b5811439ade11bb2bd471bb28 Mon Sep 17 00:00:00 2001 From: Will Harney Date: Fri, 27 Oct 2023 15:52:24 -0400 Subject: [PATCH 09/11] Use more normal callback definition. --- lib/utils.ts | 11 ++++------- lib/validators.ts | 4 ++-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/utils.ts b/lib/utils.ts index 297576ef..7c088ee8 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -1,11 +1,8 @@ -/** A callback function that expects an error to be passed. */ -export type ErrorCallback = (error: Error, result?: never) => void - -/** A callback function that expects a successful result. */ -type SuccessCallback = (error: null, result: T) => void - /** A callback function that accepts an error or a result. */ -export type Callback = SuccessCallback & ErrorCallback +export interface Callback { + (error: Error, result?: never): void + (error: null, result: T): void +} /** Safely converts any value to string, using the value's own `toString` when available. */ export const safeToString = (val: unknown) => { diff --git a/lib/validators.ts b/lib/validators.ts index cfc4d500..a705d162 100644 --- a/lib/validators.ts +++ b/lib/validators.ts @@ -26,7 +26,7 @@ SOFTWARE. ************************************************************************************ */ -import { ErrorCallback, safeToString } from './utils' +import { Callback, safeToString } from './utils' /* Validation functions copied from check-types package - https://www.npmjs.com/package/check-types */ @@ -68,7 +68,7 @@ export function isInteger(data: unknown): boolean { */ export function validate( bool: boolean, - cbOrMessage?: ErrorCallback | string, + cbOrMessage?: Callback | string, message?: string, ): void { if (bool) return // Validation passes From 7a24c79ec7425cadca310b8e54a730182315d7c1 Mon Sep 17 00:00:00 2001 From: Will Harney Date: Fri, 27 Oct 2023 16:02:12 -0400 Subject: [PATCH 10/11] Modern syntax! --- lib/memstore.ts | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/lib/memstore.ts b/lib/memstore.ts index 441b92df..cf97bf8a 100644 --- a/lib/memstore.ts +++ b/lib/memstore.ts @@ -72,42 +72,25 @@ export class MemoryCookieStore extends Store { domain: string | null, path: string | null, key: string | undefined, - ): Promise + ): Promise override findCookie( domain: string | null, path: string | null, key: string | undefined, - callback: Callback, + callback: Callback, ): void override findCookie( domain: string | null, path: string | null, key: string | undefined, - callback?: Callback, + callback?: Callback, ): unknown { const promiseCallback = createPromiseCallback(callback) - const cb = promiseCallback.callback - - if (domain == null || path == null) { - return promiseCallback.resolve(undefined) - } - - const domainEntry = this.idx[domain] - if (!domainEntry) { - return promiseCallback.resolve(undefined) - } - - const pathEntry = domainEntry[path] - if (!pathEntry) { + if (domain == null || path == null || key == null) { return promiseCallback.resolve(undefined) } - - if (key == null) { - return promiseCallback.resolve(null) - } - - cb(null, pathEntry[key] || null) - return promiseCallback.promise + const result = this.idx?.[domain]?.[path]?.[key] + return promiseCallback.resolve(result) } override findCookies( From 5220bf61833207e59c5de26da606a70e134713df Mon Sep 17 00:00:00 2001 From: Will Harney Date: Mon, 30 Oct 2023 11:40:51 -0400 Subject: [PATCH 11/11] Revert "Remove unnecessary helper." Turns out it's necessary. This reverts commit f2df0bb4ddd60d7196ac56d6194663fea71e0f4f. --- lib/utils.ts | 6 +++++- lib/validators.ts | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/utils.ts b/lib/utils.ts index 7c088ee8..effb02b2 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -4,6 +4,10 @@ export interface Callback { (error: null, result: T): void } +/** Wrapped `Object.prototype.toString`, so that you don't need to remember to use `.call()`. */ +export const objectToString = (obj: unknown) => + Object.prototype.toString.call(obj) + /** Safely converts any value to string, using the value's own `toString` when available. */ export const safeToString = (val: unknown) => { // Ideally, we'd just use String() for everything, but it breaks if `toString` is missing (mostly @@ -11,7 +15,7 @@ export const safeToString = (val: unknown) => { if (val === undefined || val === null || typeof val.toString === 'function') { return String(val) } else { - return Object.prototype.toString.call(val) + return objectToString(val) } } diff --git a/lib/validators.ts b/lib/validators.ts index a705d162..64754973 100644 --- a/lib/validators.ts +++ b/lib/validators.ts @@ -26,7 +26,7 @@ SOFTWARE. ************************************************************************************ */ -import { Callback, safeToString } from './utils' +import { Callback, objectToString, safeToString } from './utils' /* Validation functions copied from check-types package - https://www.npmjs.com/package/check-types */ @@ -52,7 +52,7 @@ export function isString(data: unknown): boolean { /** Determines whether the string representation of the argument is "[object Object]". */ export function isObject(data: unknown): boolean { - return safeToString(data) === '[object Object]' + return objectToString(data) === '[object Object]' } /** Determines whether the argument is an integer. */