clean up usage of null/undefined (#380)

* Make usage of null/undefined more consistent. Primarily allowing either value for user input. * add explicit return type to canonicalDomain * add note for weird parse behavior * Update lib/cookie/cookie.ts Co-authored-by: Colin Casey <[email protected]> * Replace `Nullable<T>` with more precise `T | undefined` * add a bit of breathing room to the file size * Change `parse` to only return undefined on failure, nut null | undefined. * standardize helpers on returning undefined * update doc for return type * change fromJSON to return undefined instead of null * fix incorrect comparison * change date helpers to avoid null, for consistency * update fromJSON tests for new return type * Make test assertions more strict Apply suggestions from code review Co-authored-by: Colin Casey <[email protected]> --------- Co-authored-by: Colin Casey <[email protected]>
salesforce · Mar 21, 2024 · a48c867 · a48c867
1 parent e2151b6
commit a48c867
Show file tree

Hide file tree

Showing 16 changed files with 204 additions and 199 deletions.
diff --git a/lib/__tests__/cookieToAndFromJson.spec.ts b/lib/__tests__/cookieToAndFromJson.spec.ts
@@ -47,7 +47,7 @@ describe('Cookie.fromJSON()', () => {
   })
 
   it('should be able to handle a null value deserialization', () => {
-    expect(Cookie.fromJSON(null)).toBeNull()
+    expect(Cookie.fromJSON(null)).toBeUndefined()
   })
 
   it('should be able to handle expiry, creation, or lastAccessed with Infinity during deserialization', () => {

diff --git a/lib/__tests__/domainMatch.spec.ts b/lib/__tests__/domainMatch.spec.ts
@@ -13,10 +13,10 @@ describe('domainMatch', () => {
     ['example.com', 'example.com.', false], // RFC6265 S4.1.2.3
 
     // nulls and undefineds
-    [null, 'example.com', null],
-    ['example.com', null, null],
-    [null, null, null],
-    [undefined, undefined, null],
+    [null, 'example.com', undefined],
+    ['example.com', null, undefined],
+    [null, null, undefined],
+    [undefined, undefined, undefined],
 
     // suffix matching:
     ['www.example.com', 'example.com', true], // substr AND suffix

diff --git a/lib/__tests__/parse.spec.ts b/lib/__tests__/parse.spec.ts
@@ -379,22 +379,22 @@ describe('Cookie.parse', () => {
     // empty string
     {
       input: ``,
-      output: null,
+      output: undefined,
     },
     // missing string
     {
       input: undefined,
-      output: null,
+      output: undefined,
     },
     // some string object
     {
       input: new String(''),
-      output: null,
+      output: undefined,
     },
     // some empty string object
     {
       input: new String(),
-      output: null,
+      output: undefined,
     },
   ])('Cookie.parse("$input")', (testCase) => {
     // Repeating the character in the input makes the jest output obnoxiously long, so instead we
@@ -406,11 +406,7 @@ describe('Cookie.parse', () => {
 
     const value = input === undefined ? undefined : input.valueOf()
     const cookie = Cookie.parse(value as string, parseOptions)
-    if (output !== undefined) {
-      expect(cookie).toEqual(output && expect.objectContaining(output))
-    } else {
-      expect(cookie).toBe(output)
-    }
+    expect(cookie).toEqual(output && expect.objectContaining(output))
 
     if (cookie && typeof assertValidateReturns === 'boolean') {
       expect(cookie.validate()).toBe(assertValidateReturns)

diff --git a/lib/cookie/canonicalDomain.ts b/lib/cookie/canonicalDomain.ts
@@ -1,10 +1,11 @@
-import * as punycode from 'punycode/punycode.js'
+import { toASCII } from 'punycode/punycode.js'
 import { IP_V6_REGEX_OBJECT } from './constants'
+import type { Nullable } from '../utils'
 
 // S5.1.2 Canonicalized Host Names
-export function canonicalDomain(str: string | null): string | null {
+export function canonicalDomain(str: Nullable<string>): string | undefined {
   if (str == null) {
-    return null
+    return undefined
   }
   let _str = str.trim().replace(/^\./, '') // S4.1.2.3 & S5.2.3: ignore leading .
 
@@ -15,7 +16,7 @@ export function canonicalDomain(str: string | null): string | null {
   // convert to IDN if any non-ASCII characters
   // eslint-disable-next-line no-control-regex
   if (/[^\u0001-\u007f]/.test(_str)) {
-    _str = punycode.toASCII(_str)
+    _str = toASCII(_str)
   }
 
   return _str.toLowerCase()

diff --git a/lib/cookie/cookie.ts b/lib/cookie/cookie.ts
@@ -30,7 +30,7 @@
  */
 
 // This file was too big before we added max-lines, and it's ongoing work to reduce its size.
-/* eslint max-lines: [1, 750] */
+/* eslint max-lines: [1, 800] */
 
 import { getPublicSuffix } from '../getPublicSuffix'
 import * as validators from '../validators'
@@ -115,12 +115,15 @@ type ParseCookieOptions = {
   loose?: boolean | undefined
 }
 
-function parse(
-  str: string,
-  options?: ParseCookieOptions,
-): Cookie | undefined | null {
+/**
+ * Parses a string into a Cookie object.
+ * @param str the Set-Cookie header or a Cookie string to parse. Note: when parsing a Cookie header it must be split by ';' before each Cookie string can be parsed.
+ * @param options configures strict or loose mode for cookie parsing
+ * @returns `Cookie` object when parsing succeeds, `undefined` when parsing fails.
+ */
+function parse(str: string, options?: ParseCookieOptions): Cookie | undefined {
   if (validators.isEmptyString(str) || !validators.isString(str)) {
-    return null
+    return undefined
   }
 
   str = str.trim()
@@ -276,17 +279,17 @@ function parse(
   return c
 }
 
-function fromJSON(str: unknown): Cookie | null {
+function fromJSON(str: unknown): Cookie | undefined {
   if (!str || validators.isEmptyString(str)) {
-    return null
+    return undefined
   }
 
   let obj: unknown
   if (typeof str === 'string') {
     try {
       obj = JSON.parse(str)
     } catch (e) {
-      return null
+      return undefined
     }
   } else {
     // assume it's an Object
@@ -528,7 +531,7 @@ export class Cookie {
     return obj
   }
 
-  clone(): Cookie | null {
+  clone(): Cookie | undefined {
     return fromJSON(this.toJSON())
   }
 
@@ -693,15 +696,12 @@ export class Cookie {
   }
 
   // Mostly S5.1.2 and S5.2.3:
-  canonicalizedDomain(): string | null {
-    if (this.domain == null) {
-      return null
-    }
+  canonicalizedDomain(): string | undefined {
     return canonicalDomain(this.domain)
   }
 
-  cdomain(): string | null {
-    return this.canonicalizedDomain()
+  cdomain(): string | undefined {
+    return canonicalDomain(this.domain)
   }
 
   static parse = parse

diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts
@@ -9,8 +9,9 @@ import { pathMatch } from '../pathMatch'
 import { Cookie } from './cookie'
 import {
   Callback,
-  createPromiseCallback,
   ErrorCallback,
+  Nullable,
+  createPromiseCallback,
   inOperator,
   safeToString,
 } from '../utils'
@@ -84,13 +85,13 @@ function getCookieContext(url: unknown): URL | urlParse<string> {
 }
 
 type SameSiteLevel = keyof (typeof Cookie)['sameSiteLevel']
-function checkSameSiteContext(value: string): SameSiteLevel | null {
+function checkSameSiteContext(value: string): SameSiteLevel | undefined {
   validators.validate(validators.isNonEmptyString(value), value)
   const context = String(value).toLowerCase()
   if (context === 'none' || context === 'lax' || context === 'strict') {
     return context
   } else {
-    return null
+    return undefined
   }
 }
 
@@ -161,7 +162,7 @@ export class CookieJar {
   readonly prefixSecurity: string
 
   constructor(
-    store?: Store | null | undefined,
+    store?: Nullable<Store>,
     options?: CreateCookieJarOptions | boolean,
   ) {
     if (typeof options === 'boolean') {
@@ -267,7 +268,7 @@ export class CookieJar {
       return promiseCallback.resolve(undefined)
     }
 
-    const host = canonicalDomain(context.hostname)
+    const host = canonicalDomain(context.hostname) ?? null
     const loose = options?.loose || this.enableLooseMode
 
     let sameSiteContext = null
@@ -440,16 +441,16 @@ export class CookieJar {
       }
     }
 
-    function withCookie(
-      err: Error | null,
-      oldCookie: Cookie | undefined | null,
+    const withCookie: Callback<Cookie | undefined> = function withCookie(
+      err,
+      oldCookie,
     ): void {
       if (err) {
         cb(err)
         return
       }
 
-      const next = function (err: Error | null): void {
+      const next: ErrorCallback = function (err) {
         if (err) {
           cb(err)
         } else if (typeof cookie === 'string') {
@@ -491,6 +492,7 @@ export class CookieJar {
       }
     }
 
+    // TODO: Refactor to avoid using a callback
     store.findCookie(cookie.domain, cookie.path, cookie.key, withCookie)
     return promiseCallback.promise
   }
@@ -748,18 +750,13 @@ export class CookieJar {
       callback,
     )
 
-    const next: Callback<Cookie[]> = function (
-      err: Error | null,
-      cookies: Cookie[] | undefined,
-    ) {
+    const next: Callback<Cookie[] | undefined> = function (err, cookies) {
       if (err) {
         promiseCallback.callback(err)
-      } else if (cookies === undefined) {
-        promiseCallback.callback(null, undefined)
       } else {
         promiseCallback.callback(
           null,
-          cookies.map((c) => {
+          cookies?.map((c) => {
             return c.toString()
           }),
         )
@@ -879,7 +876,7 @@ export class CookieJar {
 
     cookies = cookies.slice() // do not modify the original
 
-    const putNext = (err: Error | null): void => {
+    const putNext: ErrorCallback = (err) => {
       if (err) {
         return callback(err, undefined)
       }
@@ -896,7 +893,7 @@ export class CookieJar {
           return callback(e instanceof Error ? e : new Error(), undefined)
         }
 
-        if (cookie === null) {
+        if (cookie === undefined) {
           return putNext(null) // skip this cookie
         }
 
@@ -995,7 +992,8 @@ export class CookieJar {
       let completedCount = 0
       const removeErrors: Error[] = []
 
-      function removeCookieCb(removeErr: Error | null): void {
+      // TODO: Refactor to avoid using callback
+      const removeCookieCb: ErrorCallback = function removeCookieCb(removeErr) {
         if (removeErr) {
           removeErrors.push(removeErr)
         }

diff --git a/lib/cookie/defaultPath.ts b/lib/cookie/defaultPath.ts
@@ -1,12 +1,14 @@
 // RFC6265 S5.1.4 Paths and Path-Match
 
+import type { Nullable } from '../utils'
+
 /*
  * "The user agent MUST use an algorithm equivalent to the following algorithm
  * to compute the default-path of a cookie:"
  *
  * Assumption: the path (and not query part or absolute uri) is passed in.
  */
-export function defaultPath(path?: string | null): string {
+export function defaultPath(path?: Nullable<string>): string {
   // "2. If the uri-path is empty or if the first character of the uri-path is not
   // a %x2F ("/") character, output %x2F ("/") and skip the remaining steps.
   if (!path || path.slice(0, 1) !== '/') {

diff --git a/lib/cookie/domainMatch.ts b/lib/cookie/domainMatch.ts
@@ -1,3 +1,4 @@
+import type { Nullable } from '../utils'
 import { canonicalDomain } from './canonicalDomain'
 
 // Dumped from [email protected], with the following changes:
@@ -9,16 +10,16 @@ const IP_REGEX_LOWERCASE =
 
 // S5.1.3 Domain Matching
 export function domainMatch(
-  str?: string | null,
-  domStr?: string | null,
+  str?: Nullable<string>,
+  domStr?: Nullable<string>,
   canonicalize?: boolean,
-): boolean | null {
+): boolean | undefined {
   if (str == null || domStr == null) {
-    return null
+    return undefined
   }
 
-  let _str: string | null
-  let _domStr: string | null
+  let _str: Nullable<string>
+  let _domStr: Nullable<string>
 
   if (canonicalize !== false) {
     _str = canonicalDomain(str)
@@ -29,7 +30,7 @@ export function domainMatch(
   }
 
   if (_str == null || _domStr == null) {
-    return null
+    return undefined
   }
 
   /*