Repository: salesforce/tough-cookie
Pull request #381 (2024-03-13 17:56:01)

Clean up cookie creation
Currently, the props on `Cookie` all have `| undefined`, even though none of them actually ever use `undefined` as a value. If we get rid of `| undefined`, though, we get a TypeScript error complaining that the prop is not definitively assigned in the constructor. This is because we use `Object.assign` to initialize props, and I guess TypeScript just can't quite handle that. So, to get rid of `undefined` as a false possible value, we have to manually assign every prop. A bit more verbose, but not a significant change.

Additionally, the cookie constructor options are identical to the props of `Cookie`, excluding the methods, so I changed the type from explicitly listing everything to just a mapped type, for simpler maintenance. I wasn't sure if that would be a good idea, because sometimes mapped types get mangled, but the result type is still readable, so I think it's fine.

![screenshot of the computed `CreateCookieOptions` type, showing that it is reported as a simple, readable key/value object type, rather than a hard-to-understand layering of mapped types](https://github.com/salesforce/tough-cookie/assets/62956339/186fc550-eb89-4b23-b22d-e479d674626b)


nb: This was originally part of #377, but has been split out for easier review.


Patch:
diff --git a/lib/cookie/cookie.ts b/lib/cookie/cookie.ts
index 75aac27..b261192 100644
--- a/lib/cookie/cookie.ts
+++ b/lib/cookie/cookie.ts
@@ -365,6 +365,17 @@ function fromJSON(str: unknown) {
   return c
 }
 
+type CreateCookieOptions = Omit<
+  {
+    // Assume that all non-method attributes on the class can be configured, except creationIndex.
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    [K in keyof Cookie as Cookie[K] extends (...args: any[]) => any
+      ? never
+      : K]?: Cookie[K]
+  },
+  'creationIndex'
+>
+
 const cookieDefaults = {
   // the order in which the RFC has them:
   key: '',
@@ -382,46 +393,42 @@ const cookieDefaults = {
   creation: null,
   lastAccessed: null,
   sameSite: undefined,
-}
-
-type CreateCookieOptions = {
-  key?: string
-  value?: string
-  expires?: Date | 'Infinity' | null
-  maxAge?: number | 'Infinity' | '-Infinity'
-  domain?: string | null
-  path?: string | null
-  secure?: boolean
-  httpOnly?: boolean
-  extensions?: string[] | null
-  creation?: Date | 'Infinity' | null
-  creationIndex?: number
-  hostOnly?: boolean | null
-  pathIsDefault?: boolean | null
-  lastAccessed?: Date | 'Infinity' | null
-  sameSite?: string | undefined
-}
+} as const satisfies Required<CreateCookieOptions>
 
 export class Cookie {
-  key: string | undefined
-  value: string | undefined
-  expires: Date | 'Infinity' | null | undefined
-  maxAge: number | 'Infinity' | '-Infinity' | undefined
-  domain: string | null | undefined
-  path: string | null | undefined
-  secure: boolean | undefined
-  httpOnly: boolean | undefined
-  extensions: string[] | null | undefined
+  key: string
+  value: string
+  expires: Date | 'Infinity' | null
+  maxAge: number | 'Infinity' | '-Infinity' | null
+  domain: string | null
+  path: string | null
+  secure: boolean
+  httpOnly: boolean
+  extensions: string[] | null
   creation: Date | 'Infinity' | null
-  creationIndex: number | undefined
-  hostOnly: boolean | null | undefined
-  pathIsDefault: boolean | null | undefined
-  lastAccessed: Date | 'Infinity' | null | undefined
+  creationIndex: number
+  hostOnly: boolean | null
+  pathIsDefault: boolean | null
+  lastAccessed: Date | 'Infinity' | null
   sameSite: string | undefined
 
   constructor(options: CreateCookieOptions = {}) {
-    Object.assign(this, cookieDefaults, options)
-    this.creation = options.creation ?? cookieDefaults.creation ?? new Date()
+    this.key = options.key ?? cookieDefaults.key
+    this.value = options.value ?? cookieDefaults.value
+    this.expires = options.expires ?? cookieDefaults.expires
+    this.maxAge = options.maxAge ?? cookieDefaults.maxAge
+    this.domain = options.domain ?? cookieDefaults.domain
+    this.path = options.path ?? cookieDefaults.path
+    this.secure = options.secure ?? cookieDefaults.secure
+    this.httpOnly = options.httpOnly ?? cookieDefaults.httpOnly
+    this.extensions = options.extensions ?? cookieDefaults.extensions
+    this.creation = options.creation ?? cookieDefaults.creation
+    this.hostOnly = options.hostOnly ?? cookieDefaults.hostOnly
+    this.pathIsDefault = options.pathIsDefault ?? cookieDefaults.pathIsDefault
+    this.lastAccessed = options.lastAccessed ?? cookieDefaults.lastAccessed
+    this.sameSite = options.sameSite ?? cookieDefaults.sameSite
+
+    this.creation = options.creation ?? new Date()
 
     // used to break creation ties in cookieCompare():
     Object.defineProperty(this, 'creationIndex', {
@@ -430,6 +437,8 @@ export class Cookie {
       writable: true,
       value: ++Cookie.cookiesCreated,
     })
+    // Duplicate operation, but it makes TypeScript happy...
+    this.creationIndex = Cookie.cookiesCreated
   }
 
   [Symbol.for('nodejs.util.inspect.custom')]() {
diff --git a/package.json b/package.json
index f9faa46..caf37ae 100644
--- a/package.json
+++ b/package.json
@@ -120,8 +120,8 @@
     "vows": "^0.8.3"
   },
   "dependencies": {
-    "tldts": "^6.1.0",
     "punycode": "^2.3.1",
+    "tldts": "^6.1.0",
     "url-parse": "^1.5.10"
   }
 }


Test patch:
diff --git a/lib/__tests__/cookieJar.spec.ts b/lib/__tests__/cookieJar.spec.ts
index 474448b..03b1648 100644
--- a/lib/__tests__/cookieJar.spec.ts
+++ b/lib/__tests__/cookieJar.spec.ts
@@ -420,7 +420,7 @@ describe('CookieJar', () => {
         expect(allHaveRootPath).toBe(true)
 
         const noCookiesWithAnOtherKeyRetrieved = cookies.every(
-          (cookie) => !/^other/.test(cookie.key as string),
+          (cookie) => !/^other/.test(cookie.key),
         )
         expect(noCookiesWithAnOtherKeyRetrieved).toBe(true)
       })
@@ -430,7 +430,7 @@ describe('CookieJar', () => {
         expect(cookies).toHaveLength(4)
 
         const noCookiesWithAnOtherKeyRetrieved = cookies.every(
-          (cookie) => !/^other/.test(cookie.key as string),
+          (cookie) => !/^other/.test(cookie.key),
         )
         expect(noCookiesWithAnOtherKeyRetrieved).toBe(true)
       })
@@ -442,7 +442,7 @@ describe('CookieJar', () => {
         expect(cookies).toHaveLength(4)
 
         const noCookiesWithAnOtherKeyRetrieved = cookies.every(
-          (cookie) => !/^other/.test(cookie.key as string),
+          (cookie) => !/^other/.test(cookie.key),
         )
         expect(noCookiesWithAnOtherKeyRetrieved).toBe(true)
       })
diff --git a/lib/__tests__/cookieSorting.spec.ts b/lib/__tests__/cookieSorting.spec.ts
index 784457c..b199789 100644
--- a/lib/__tests__/cookieSorting.spec.ts
+++ b/lib/__tests__/cookieSorting.spec.ts
@@ -9,9 +9,7 @@ describe('Cookie sorting', () => {
       const cookie2 = new Cookie()
       expect(typeof cookie1.creationIndex).toBe('number')
       expect(typeof cookie2.creationIndex).toBe('number')
-      expect(cookie1.creationIndex).toBeLessThan(
-        cookie2.creationIndex as number,
-      )
+      expect(cookie1.creationIndex).toBeLessThan(cookie2.creationIndex)
     })
 
     it('should set the creation index during construction when creation time is provided', () => {
@@ -21,9 +19,7 @@ describe('Cookie sorting', () => {
       expect(cookie1.creation).toEqual(cookie2.creation)
       expect(typeof cookie1.creationIndex).toBe('number')
       expect(typeof cookie2.creationIndex).toBe('number')
-      expect(cookie1.creationIndex).toBeLessThan(
-        cookie2.creationIndex as number,
-      )
+      expect(cookie1.creationIndex).toBeLessThan(cookie2.creationIndex)
     })
 
     it('should leave the creation index alone during setCookie', async () => {