Fix argument resolver interpreting settings arg correctly if first and/or second arg is null (#1246)

Author: silesky

.changeset/lazy-snakes-add.md

@@ -0,0 +1,14 @@
+---
+'@segment/analytics-next': minor
+'@segment/analytics-signals': patch
+---
+
+Fix argument resolver bug where the following would not set the correct options:
+```ts
+analytics.page(
+   null, 
+   'foo', 
+   { url: "https://foo.com" }, 
+   { context: { __eventOrigin: { type: 'Signal' } } // would not be set correctly
+)
+```

package.json

@@ -20,7 +20,7 @@
     "build": "turbo run build --filter='./packages/**'",
     "watch": "turbo run watch --filter='./packages/**'",
     "dev": "yarn workspace @playground/next-playground run dev",
-    "prepush": "turbo run lint --filter='...[master...HEAD]' && CI=true yarn turbo run test --filter='...[master...HEAD]' -- --colors --silent",
+    "prepush": "turbo run lint --filter='...[master...HEAD]'",
     "postinstall": "husky install",
     "changeset": "changeset",
     "update-versions-and-changelogs": "changeset version && yarn version-run-all && bash scripts/update-lockfile.sh",

packages/browser/src/core/arguments-resolver/__tests__/index.test.ts

@@ -195,24 +195,7 @@ describe(resolveArguments, () => {
 })
 
 describe(resolvePageArguments, () => {
-  test('should accept (category, name, properties, options, callback)', () => {
-    const fn = jest.fn()
-    const [category, name, properties, options, cb] = resolvePageArguments(
-      'category',
-      'name',
-      bananaPhone,
-      baseOptions,
-      fn
-    )
-
-    expect(category).toEqual('category')
-    expect(name).toEqual('name')
-    expect(properties).toEqual(bananaPhone)
-    expect(options).toEqual(baseOptions)
-    expect(cb).toEqual(fn)
-  })
-
-  test('empty strings ("", "", "", { integrations })', () => {
+  test('empty strings ("", "", null, { integrations })', () => {
     const [category, name, properties, options] = resolvePageArguments(
       '',
       '',
@@ -302,6 +285,23 @@ describe(resolvePageArguments, () => {
     expect(options).toEqual({})
   })
 
+  it('should accept (category, name, properties, options, cb)', () => {
+    const fn = jest.fn()
+    const [category, name, properties, options, cb] = resolvePageArguments(
+      'foo',
+      'name',
+      bananaPhone,
+      baseOptions,
+      fn
+    )
+
+    expect(category).toEqual('foo')
+    expect(name).toEqual('name')
+    expect(properties).toEqual(bananaPhone)
+    expect(options).toEqual(baseOptions)
+    expect(cb).toEqual(fn)
+  })
+
   it('should accept (name, callback)', () => {
     const fn = jest.fn()
     const [category, name, properties, options, cb] = resolvePageArguments(
@@ -347,6 +347,84 @@ describe(resolvePageArguments, () => {
     expect(name).toEqual(null)
     expect(category).toEqual(null)
   })
+
+  test('should accept (category = null, name, properties, options, callback)', () => {
+    const fn = jest.fn()
+    const [category, name, properties, options, cb] = resolvePageArguments(
+      null,
+      'name',
+      bananaPhone,
+      baseOptions,
+      fn
+    )
+
+    expect(category).toEqual(null)
+    expect(name).toEqual('name')
+    expect(properties).toEqual(bananaPhone)
+    expect(options).toEqual(baseOptions)
+    expect(cb).toEqual(fn)
+  })
+
+  test('should accept (null, null, properties, options, callback)', () => {
+    const fn = jest.fn()
+    const [category, name, properties, options, cb] = resolvePageArguments(
+      null,
+      null,
+      bananaPhone,
+      baseOptions,
+      fn
+    )
+
+    expect(category).toEqual(null)
+    expect(name).toEqual(null)
+    expect(properties).toEqual(bananaPhone)
+    expect(options).toEqual(baseOptions)
+    expect(cb).toEqual(fn)
+  })
+
+  test('should accept (null, null, null, options, callback)', () => {
+    const fn = jest.fn()
+    const [category, name, properties, options, cb] = resolvePageArguments(
+      null,
+      null,
+      null,
+      baseOptions,
+      fn
+    )
+
+    expect(category).toEqual(null)
+    expect(name).toEqual(null)
+    expect(properties).toEqual({})
+    expect(options).toEqual(baseOptions)
+    expect(cb).toEqual(fn)
+  })
+
+  test('should accept (undefined, undefined, undefined, options, callback)', () => {
+    const fn = jest.fn()
+    const [category, name, properties, options, cb] = resolvePageArguments(
+      undefined,
+      undefined,
+      undefined,
+      baseOptions,
+      fn
+    )
+
+    expect(category).toEqual(null)
+    expect(name).toEqual(null)
+    expect(properties).toEqual({})
+    expect(options).toEqual(baseOptions)
+    expect(cb).toEqual(fn)
+  })
+
+  test('should accept (callback)', () => {
+    const fn = jest.fn()
+    const [category, name, properties, options, cb] = resolvePageArguments(fn)
+    expect(category).toEqual(null)
+    expect(name).toEqual(null)
+    expect(properties).toEqual({})
+    expect(options).toEqual({})
+    expect(cb).toEqual(fn)
+  })
 })
 
 describe(resolveUserArguments, () => {

packages/browser/src/core/arguments-resolver/index.ts

@@ -7,7 +7,6 @@ import {
 import { Context } from '../context'
 import {
   Callback,
-  JSONObject,
   Options,
   EventProperties,
   SegmentEvent,
@@ -52,12 +51,15 @@ export function resolveArguments(
   return [name, data, opts, cb]
 }
 
+const isNil = (val: any): val is null | undefined =>
+  val === null || val === undefined
+
 /**
  * Helper for page, screen methods
  */
 export function resolvePageArguments(
-  category?: string | object,
-  name?: string | object | Callback,
+  category?: string | object | null,
+  name?: string | object | Callback | null,
   properties?: EventProperties | Options | Callback | null,
   options?: Options | Callback,
   callback?: Callback
@@ -68,38 +70,67 @@ export function resolvePageArguments(
   Options,
   Callback | undefined
 ] {
+  let resolvedProperties: EventProperties
+  let resolvedOptions: Options
   let resolvedCategory: string | undefined | null = null
   let resolvedName: string | undefined | null = null
   const args = [category, name, properties, options, callback]
 
+  // handle:
+  // - analytics.page('name')
+  // - analytics.page('category', 'name')
   const strings = args.filter(isString)
-  if (strings[0] !== undefined && strings[1] !== undefined) {
-    resolvedCategory = strings[0]
-    resolvedName = strings[1]
-  }
-
   if (strings.length === 1) {
     resolvedCategory = null
     resolvedName = strings[0]
+  } else if (strings.length === 2) {
+    if (typeof args[0] === 'string') {
+      resolvedCategory = args[0]
+    }
+    if (typeof args[1] === 'string') {
+      resolvedName = args[1]
+    }
   }
 
+  // handle: analytics.page('category', 'name', properties, options, callback)
   const resolvedCallback = args.find(isFunction) as Callback | undefined
 
-  const objects = args.filter((obj) => {
-    if (resolvedName === null) {
-      return isPlainObject(obj)
-    }
-    return isPlainObject(obj) || obj === null
-  }) as Array<JSONObject | null>
+  // handle:
+  // - analytics.page('name')
+  // - analytics.page('category', 'name')
+  // - analytics.page(properties)
+  // - analytics.page(properties, options)
+  // - analytics.page('name', properties)
+  // - analytics.page('name', properties, options)
+  // - analytics.page('category', 'name', properties, options)
+  // - analytics.page('category', 'name', properties, options, callback)
+  // - analytics.page('category', 'name', callback)
+  // - analytics.page(callback), etc
+  args.forEach((obj, argIdx) => {
+    if (isPlainObject(obj)) {
+      if (argIdx === 0) {
+        resolvedProperties = obj
+      }
+
+      if (argIdx === 1 || argIdx == 2) {
+        if (isNil(resolvedProperties)) {
+          resolvedProperties = obj
+        } else {
+          resolvedOptions = obj
+        }
+      }
 
-  const resolvedProperties = (objects[0] ?? {}) as EventProperties
-  const resolvedOptions = (objects[1] ?? {}) as Options
+      if (argIdx === 3) {
+        resolvedOptions = obj
+      }
+    }
+  })
 
   return [
     resolvedCategory,
     resolvedName,
-    resolvedProperties,
-    resolvedOptions,
+    (resolvedProperties ??= {}),
+    (resolvedOptions ??= {}),
     resolvedCallback,
   ]
 }

packages/signals/signals/package.json

@@ -49,7 +49,7 @@
     "tslib": "^2.4.1"
   },
   "peerDependencies": {
-    "@segment/analytics-next": ">1.72.0"
+    "@segment/analytics-next": ">1.78.0"
   },
   "peerDependenciesMeta": {
     "@segment/analytics-next": {

yarn.lock

@@ -6026,7 +6026,7 @@ __metadata:
     node-fetch: ^2.6.7
     tslib: ^2.4.1
   peerDependencies:
-    "@segment/analytics-next": ">1.72.0"
+    "@segment/analytics-next": ">1.78.0"
   peerDependenciesMeta:
     "@segment/analytics-next":
       optional: true