fix / refactor

Author: silesky

.changeset/fast-lions-kick.md

@@ -0,0 +1,5 @@
+---
+'@segment/analytics-next': patch
+---
+
+Add support for more argument resolver edge cases

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

@@ -3,7 +3,7 @@ import {
   resolvePageArguments,
   resolveUserArguments,
   resolveAliasArguments,
-} from '../'
+} from '../index'
 import { Callback } from '../../events'
 import { User } from '../../user'
 
@@ -195,7 +195,7 @@ describe(resolveArguments, () => {
 })
 
 describe(resolvePageArguments, () => {
-  test('empty strings ("", "", null, { integrations })', () => {
+  test('empty strings ("", "", {}, { integrations })', () => {
     const [category, name, properties, options] = resolvePageArguments(
       '',
       '',
@@ -211,14 +211,44 @@ describe(resolvePageArguments, () => {
 
     expect(category).toEqual('')
     expect(name).toEqual('')
-    expect(properties).toEqual({})
     expect(options).toEqual({
       integrations: {
         Amplitude: {
           sessionId: '123',
         },
       },
     })
+    expect(properties).toEqual({})
+  })
+
+  it('should accept (category, name)', () => {
+    const [category, name, properties, options, cb] = resolvePageArguments(
+      'category',
+      'name'
+    )
+
+    expect(name).toEqual('name')
+    expect(category).toEqual('category')
+    expect(properties).toEqual({})
+    expect(options).toEqual({})
+    expect(cb).toEqual(undefined)
+  })
+
+  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)
   })
 
   test('should accept (category, name, properties, callback)', () => {
@@ -254,52 +284,48 @@ describe(resolvePageArguments, () => {
     expect(options).toEqual({})
   })
 
-  it('should accept (name, properties, options, callback)', () => {
-    const fn = jest.fn()
+  it('should accept (name, properties)', () => {
     const [category, name, properties, options, cb] = resolvePageArguments(
       'name',
-      bananaPhone,
-      baseOptions,
-      fn
+      bananaPhone
     )
 
-    expect(category).toEqual(null)
     expect(name).toEqual('name')
+    expect(category).toEqual(null)
     expect(properties).toEqual(bananaPhone)
-    expect(options).toEqual(baseOptions)
-    expect(cb).toEqual(fn)
+    expect(options).toEqual({})
+    expect(cb).toEqual(undefined)
   })
 
-  it('should accept (name, properties, callback)', () => {
+  it('should accept (name, properties, options, callback)', () => {
     const fn = jest.fn()
     const [category, name, properties, options, cb] = resolvePageArguments(
       'name',
       bananaPhone,
+      baseOptions,
       fn
     )
 
     expect(category).toEqual(null)
     expect(name).toEqual('name')
     expect(properties).toEqual(bananaPhone)
+    expect(options).toEqual(baseOptions)
     expect(cb).toEqual(fn)
-    expect(options).toEqual({})
   })
 
-  it('should accept (category, name, properties, options, cb)', () => {
+  it('should accept (name, properties, callback)', () => {
     const fn = jest.fn()
     const [category, name, properties, options, cb] = resolvePageArguments(
-      'foo',
       'name',
       bananaPhone,
-      baseOptions,
       fn
     )
 
-    expect(category).toEqual('foo')
+    expect(category).toEqual(null)
     expect(name).toEqual('name')
     expect(properties).toEqual(bananaPhone)
-    expect(options).toEqual(baseOptions)
     expect(cb).toEqual(fn)
+    expect(options).toEqual({})
   })
 
   it('should accept (name, callback)', () => {
@@ -348,7 +374,7 @@ describe(resolvePageArguments, () => {
     expect(category).toEqual(null)
   })
 
-  test('should accept (category = null, name, properties, options, callback)', () => {
+  test('should accept (null, name, properties, options, callback)', () => {
     const fn = jest.fn()
     const [category, name, properties, options, cb] = resolvePageArguments(
       null,
@@ -365,6 +391,64 @@ describe(resolvePageArguments, () => {
     expect(cb).toEqual(fn)
   })
 
+  test('should accept (name, null, properties, options, callback)', () => {
+    const fn = jest.fn()
+    const [category, name, properties, options, cb] = resolvePageArguments(
+      'name',
+      null,
+      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 (name, null, properties)', () => {
+    const [category, name, properties, options] = resolvePageArguments(
+      'name',
+      null,
+      bananaPhone
+    )
+
+    expect(name).toEqual('name')
+    expect(category).toEqual(null)
+    expect(properties).toEqual(bananaPhone)
+    expect(options).toEqual({})
+  })
+
+  test('should accept (name, null, null, options)', () => {
+    const [category, name, properties, options] = resolvePageArguments(
+      'name',
+      null,
+      null,
+      baseOptions
+    )
+
+    expect(name).toEqual('name')
+    expect(category).toEqual(null)
+    expect(properties).toEqual({})
+    expect(options).toEqual(baseOptions)
+  })
+
+  test('should accept (null, null, properties)', () => {
+    const [category, name, properties, options, cb] = resolvePageArguments(
+      null,
+      null,
+      bananaPhone
+    )
+
+    expect(category).toEqual(null)
+    expect(name).toEqual(null)
+    expect(properties).toEqual(bananaPhone)
+    expect(options).toEqual({})
+    expect(cb).toEqual(undefined)
+  })
+
   test('should accept (null, null, properties, options, callback)', () => {
     const fn = jest.fn()
     const [category, name, properties, options, cb] = resolvePageArguments(
@@ -425,6 +509,16 @@ describe(resolvePageArguments, () => {
     expect(options).toEqual({})
     expect(cb).toEqual(fn)
   })
+
+  test('should accept (null, null, null, null, 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

@@ -51,9 +51,6 @@ 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
  */
@@ -76,13 +73,18 @@ export function resolvePageArguments(
   let resolvedName: string | undefined | null = null
   const args = [category, name, properties, options, callback]
 
-  // handle:
-  // - analytics.page('name')
-  // - analytics.page('category', 'name')
+  // The legacy logic is basically:
+  // - If there is a string, it's the name
+  // - If there are two strings, it's category and name
   const strings = args.filter(isString)
   if (strings.length === 1) {
-    resolvedCategory = null
-    resolvedName = strings[0]
+    if (isString(args[1])) {
+      resolvedName = args[1]
+      resolvedCategory = null
+    } else {
+      resolvedName = strings[0]
+      resolvedCategory = null
+    }
   } else if (strings.length === 2) {
     if (typeof args[0] === 'string') {
       resolvedCategory = args[0]
@@ -106,25 +108,26 @@ export function resolvePageArguments(
   // - 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
-        }
-      }
 
-      if (argIdx === 3) {
-        resolvedOptions = obj
-      }
+  // The legacy logic is basically:
+  // - If there is a plain object, it's the properties
+  // - If there are two plain objects, it's properties and options
+  const objects = args.filter(isPlainObject)
+  if (objects.length === 1) {
+    if (isPlainObject(args[2])) {
+      resolvedOptions = {}
+      resolvedProperties = args[2]
+    } else if (isPlainObject(args[3])) {
+      resolvedProperties = {}
+      resolvedOptions = args[3]
+    } else {
+      resolvedProperties = objects[0]
+      resolvedOptions = {}
     }
-  })
+  } else if (objects.length === 2) {
+    resolvedProperties = objects[0]
+    resolvedOptions = objects[1]
+  }
 
   return [
     resolvedCategory,