From d0f425bbe375fb8f93ac0fdc2a5018edfcf76fac Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Mar 2025 21:29:33 -0600 Subject: [PATCH 1/7] fix argument resolver null issue --- .../__tests__/index.test.ts | 104 +++++++++++++++--- .../src/core/arguments-resolver/index.ts | 51 ++++++--- 2 files changed, 122 insertions(+), 33 deletions(-) diff --git a/packages/browser/src/core/arguments-resolver/__tests__/index.test.ts b/packages/browser/src/core/arguments-resolver/__tests__/index.test.ts index 2bc6f6572..7e400213c 100644 --- a/packages/browser/src/core/arguments-resolver/__tests__/index.test.ts +++ b/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,74 @@ 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) + }) }) describe(resolveUserArguments, () => { diff --git a/packages/browser/src/core/arguments-resolver/index.ts b/packages/browser/src/core/arguments-resolver/index.ts index c581898c2..a8c4d54a9 100644 --- a/packages/browser/src/core/arguments-resolver/index.ts +++ b/packages/browser/src/core/arguments-resolver/index.ts @@ -52,12 +52,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 +71,56 @@ 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] - const strings = args.filter(isString) - if (strings[0] !== undefined && strings[1] !== undefined) { - resolvedCategory = strings[0] - resolvedName = strings[1] + if (typeof args[0] === 'string') { + resolvedCategory = args[0] + } + if (typeof args[1] === 'string') { + resolvedName = args[1] } + // if there is just one string in the first two args, it's the name + const strings = [args[0], args[1]].filter(isString) if (strings.length === 1) { resolvedCategory = null resolvedName = strings[0] } + // if there is any function, it's always the callback const resolvedCallback = args.find(isFunction) as Callback | undefined - const objects = args.filter((obj) => { - if (resolvedName === null) { - return isPlainObject(obj) + 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 it's the third argument and it's an object, it's always properties + if (argIdx === 3) { + resolvedOptions = obj + } } - return isPlainObject(obj) || obj === null - }) as Array + }) - const resolvedProperties = (objects[0] ?? {}) as EventProperties - const resolvedOptions = (objects[1] ?? {}) as Options + // if there is an object and it's the fourth argument, it's options return [ resolvedCategory, resolvedName, - resolvedProperties, - resolvedOptions, + (resolvedProperties ??= {}), + (resolvedOptions ??= {}), resolvedCallback, ] } From 432fa1c27d49c073f46594075964e2d26e3b3af4 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Mar 2025 21:42:27 -0600 Subject: [PATCH 2/7] add change --- packages/browser/src/core/arguments-resolver/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/browser/src/core/arguments-resolver/index.ts b/packages/browser/src/core/arguments-resolver/index.ts index a8c4d54a9..8fb7d3f2e 100644 --- a/packages/browser/src/core/arguments-resolver/index.ts +++ b/packages/browser/src/core/arguments-resolver/index.ts @@ -7,7 +7,6 @@ import { import { Context } from '../context' import { Callback, - JSONObject, Options, EventProperties, SegmentEvent, From 58e6cbcc672dc8548c3d7ac3a8e710e12983c4ec Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Mar 2025 21:51:59 -0600 Subject: [PATCH 3/7] fix event signals resolution bug --- .changeset/lazy-snakes-add.md | 14 ++++++++++++++ packages/signals/signals/package.json | 2 +- yarn.lock | 2 +- 3 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 .changeset/lazy-snakes-add.md diff --git a/.changeset/lazy-snakes-add.md b/.changeset/lazy-snakes-add.md new file mode 100644 index 000000000..b5a4886ea --- /dev/null +++ b/.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 +) +``` diff --git a/packages/signals/signals/package.json b/packages/signals/signals/package.json index f13827912..37523e909 100644 --- a/packages/signals/signals/package.json +++ b/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": { diff --git a/yarn.lock b/yarn.lock index 1159d9015..7fc0a637a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6020,7 +6020,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 From b6704cfad406c3c7d91cc652699eb8e3bc5211f4 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Mar 2025 22:09:15 -0600 Subject: [PATCH 4/7] wip --- packages/browser/src/core/arguments-resolver/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/browser/src/core/arguments-resolver/index.ts b/packages/browser/src/core/arguments-resolver/index.ts index 8fb7d3f2e..97303516e 100644 --- a/packages/browser/src/core/arguments-resolver/index.ts +++ b/packages/browser/src/core/arguments-resolver/index.ts @@ -98,6 +98,7 @@ export function resolvePageArguments( if (argIdx === 0) { resolvedProperties = obj } + if (argIdx === 1 || argIdx == 2) { if (isNil(resolvedProperties)) { resolvedProperties = obj @@ -106,15 +107,12 @@ export function resolvePageArguments( } } - // if it's the third argument and it's an object, it's always properties if (argIdx === 3) { resolvedOptions = obj } } }) - // if there is an object and it's the fourth argument, it's options - return [ resolvedCategory, resolvedName, From b17f6e5fa498125ef262133717aaf7997264f21e Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Mar 2025 22:15:49 -0600 Subject: [PATCH 5/7] update --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 1d9df61f1..95fee3480 100644 --- a/package.json +++ b/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", From dd4b14d2179f6b20dda80fd506f3c0e41873112b Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Mar 2025 22:39:25 -0600 Subject: [PATCH 6/7] wip --- .../src/core/arguments-resolver/index.ts | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/browser/src/core/arguments-resolver/index.ts b/packages/browser/src/core/arguments-resolver/index.ts index 97303516e..98df8ccf5 100644 --- a/packages/browser/src/core/arguments-resolver/index.ts +++ b/packages/browser/src/core/arguments-resolver/index.ts @@ -76,23 +76,31 @@ export function resolvePageArguments( let resolvedName: string | undefined | null = null const args = [category, name, properties, options, callback] - if (typeof args[0] === 'string') { - resolvedCategory = args[0] - } - if (typeof args[1] === 'string') { - resolvedName = args[1] - } - - // if there is just one string in the first two args, it's the name - const strings = [args[0], args[1]].filter(isString) + // handle: + // - analytics.page('name') + // - analytics.page('category', 'name') + const strings = args.filter(isString) 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] + } } - // if there is any function, it's always the callback + // handle: analytics.page('category', 'name', properties, options, callback) const resolvedCallback = args.find(isFunction) as Callback | undefined + // handle: + // - analytics.page(properties) + // - analytics.page(properties, options) + // - analytics.page('name', properties) + // - analytics.page('name', properties, options) + // - analytics.page('category', 'name', properties, options) args.forEach((obj, argIdx) => { if (isPlainObject(obj)) { if (argIdx === 0) { From 8d224ab5123f7cd91b5f229478e84bc23061cfb0 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Mar 2025 23:09:41 -0600 Subject: [PATCH 7/7] wip --- .../core/arguments-resolver/__tests__/index.test.ts | 10 ++++++++++ packages/browser/src/core/arguments-resolver/index.ts | 5 +++++ 2 files changed, 15 insertions(+) diff --git a/packages/browser/src/core/arguments-resolver/__tests__/index.test.ts b/packages/browser/src/core/arguments-resolver/__tests__/index.test.ts index 7e400213c..e6b169be0 100644 --- a/packages/browser/src/core/arguments-resolver/__tests__/index.test.ts +++ b/packages/browser/src/core/arguments-resolver/__tests__/index.test.ts @@ -415,6 +415,16 @@ describe(resolvePageArguments, () => { 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, () => { diff --git a/packages/browser/src/core/arguments-resolver/index.ts b/packages/browser/src/core/arguments-resolver/index.ts index 98df8ccf5..b5c35fe4d 100644 --- a/packages/browser/src/core/arguments-resolver/index.ts +++ b/packages/browser/src/core/arguments-resolver/index.ts @@ -96,11 +96,16 @@ export function resolvePageArguments( const resolvedCallback = args.find(isFunction) as Callback | undefined // 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) {