From cc06181acbe1a4b5ca74335271babd46222cb02d Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Mon, 17 Apr 2023 14:56:31 -0500 Subject: [PATCH 1/4] fix late campaign parsing --- .../segmentio/__tests__/normalize.test.ts | 80 ++++++++----------- .../src/plugins/segmentio/normalize.ts | 7 +- 2 files changed, 39 insertions(+), 48 deletions(-) diff --git a/packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts b/packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts index 5e83069f5..9f4835bdd 100644 --- a/packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts +++ b/packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts @@ -51,13 +51,24 @@ describe('before loading', () => { window.localStorage.clear() } }) + describe('#normalize', () => { let object: SegmentEvent + let defaultCtx: any + const withSearchParams = (search?: string) => { + object.context = { page: { search } } + } beforeEach(() => { cookie.remove('s:context.referrer') + defaultCtx = { + page: { + search: '', + }, + } object = { type: 'track', + context: defaultCtx, } }) @@ -94,17 +105,15 @@ describe('before loading', () => { }) it('should always add .anonymousId even if .userId is given', () => { - const object: SegmentEvent = { userId: 'baz', type: 'track' } + object.userId = 'baz' normalize(analytics, object, options, {}) assert(object.anonymousId?.length === 36) }) it('should accept anonymousId being set in an event', async () => { - const object: SegmentEvent = { - userId: 'baz', - type: 'track', - anonymousId: '👻', - } + object.userId = 'baz' + object.anonymousId = '👻' + normalize(analytics, object, options, {}) expect(object.anonymousId).toEqual('👻') }) @@ -115,15 +124,16 @@ describe('before loading', () => { }) it('should not rewrite context if provided', () => { - const ctx = {} + const ctx = defaultCtx const obj = { ...object, context: ctx } normalize(analytics, obj, options, {}) expect(obj.context).toEqual(ctx) }) - it('should copy .options to .context', () => { + it('should overwrite options with context if context does not exist', () => { const opts = {} const obj = { ...object, options: opts } + delete obj.context normalize(analytics, obj, options, {}) assert(obj.context === opts) assert(obj.options == null) @@ -172,6 +182,7 @@ describe('before loading', () => { it('should not replace .locale if provided', () => { const ctx = { + ...defaultCtx, locale: 'foobar', } const obj = { ...object, context: ctx } @@ -180,10 +191,9 @@ describe('before loading', () => { }) it('should add .campaign', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm_source=source&utm_medium=medium&utm_term=term&utm_content=content&utm_campaign=name', - }) - + withSearchParams( + 'utm_source=source&utm_medium=medium&utm_term=term&utm_content=content&utm_campaign=name' + ) normalize(analytics, object, options, {}) assert(object) @@ -197,10 +207,7 @@ describe('before loading', () => { }) it('should decode query params', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm_source=%5BFoo%5D', - }) - + withSearchParams('?utm_source=%5BFoo%5D') normalize(analytics, object, options, {}) assert(object) @@ -210,9 +217,7 @@ describe('before loading', () => { }) it('should guard against undefined utm params', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm_source', - }) + withSearchParams('?utm_source') normalize(analytics, object, options, {}) @@ -223,10 +228,7 @@ describe('before loading', () => { }) it('should guard against empty utm params', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm_source=', - }) - + withSearchParams('?utm_source=') normalize(analytics, object, options, {}) assert(object) @@ -236,10 +238,7 @@ describe('before loading', () => { }) it('only parses utm params suffixed with _', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm', - }) - + withSearchParams('?utm') normalize(analytics, object, options, {}) assert(object) @@ -248,9 +247,7 @@ describe('before loading', () => { }) it('should guard against short utm params', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm_', - }) + withSearchParams('?utm_') normalize(analytics, object, options, {}) @@ -260,13 +257,14 @@ describe('before loading', () => { }) it('should allow override of .campaign', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm_source=source&utm_medium=medium&utm_term=term&utm_content=content&utm_campaign=name', - }) + withSearchParams( + '?utm_source=source&utm_medium=medium&utm_term=term&utm_content=content&utm_campaign=name' + ) const obj = { ...object, context: { + ...defaultCtx, campaign: { source: 'overrideSource', medium: 'overrideMedium', @@ -288,9 +286,7 @@ describe('before loading', () => { }) it('should add .referrer.id and .referrer.type (cookies)', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm_source=source&urid=medium', - }) + withSearchParams('?utm_source=source&urid=medium') normalize(analytics, object, options, {}) assert(object) @@ -307,9 +303,7 @@ describe('before loading', () => { }) it('should add .referrer.id and .referrer.type (cookieless)', () => { - jsdom.reconfigure({ - url: 'http://localhost?utm_source=source&urid=medium', - }) + withSearchParams('utm_source=source&urid=medium') const setCookieSpy = jest.spyOn(cookie, 'set') analytics = new Analytics( { writeKey: options.apiKey }, @@ -329,10 +323,6 @@ describe('before loading', () => { it('should add .referrer.id and .referrer.type from cookie', () => { cookie.set('s:context.referrer', '{"id":"baz","type":"millennial-media"}') - jsdom.reconfigure({ - url: 'http://localhost?utm_source=source', - }) - normalize(analytics, object, options, {}) assert(object) @@ -348,10 +338,6 @@ describe('before loading', () => { '{"id":"medium","type":"millennial-media"}' ) - jsdom.reconfigure({ - url: 'http://localhost', - }) - normalize(analytics, object, options, {}) assert(object) assert(object.context) diff --git a/packages/browser/src/plugins/segmentio/normalize.ts b/packages/browser/src/plugins/segmentio/normalize.ts index b913c723b..f02067943 100644 --- a/packages/browser/src/plugins/segmentio/normalize.ts +++ b/packages/browser/src/plugins/segmentio/normalize.ts @@ -125,11 +125,16 @@ export function normalize( integrations?: LegacySettings['integrations'] ): object { const user = analytics.user() - const query = window.location.search json.context = json.context ?? json.options ?? {} const ctx = json.context + // This guard should not be neccessary -- why would context not exist here? Ditto ^ -- + // page enrichment should add a context to an event by default. + // In any case, adding an empty string 'default'. + // We do not use the current search parameters, as they might be stale by this point. + const query: string = ctx.page?.search || '' + delete json.options json.writeKey = settings?.apiKey ctx.userAgent = window.navigator.userAgent From 044149656f69bcd284188bf43673b9aefb6f5d9e Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Mon, 17 Apr 2023 16:03:47 -0500 Subject: [PATCH 2/4] revise comments --- packages/browser/src/plugins/segmentio/normalize.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/browser/src/plugins/segmentio/normalize.ts b/packages/browser/src/plugins/segmentio/normalize.ts index f02067943..af75c2c6a 100644 --- a/packages/browser/src/plugins/segmentio/normalize.ts +++ b/packages/browser/src/plugins/segmentio/normalize.ts @@ -126,13 +126,11 @@ export function normalize( ): object { const user = analytics.user() + // context should always exist here (see page enrichment)? ... and why would we default to json.options? todo: delete this json.context = json.context ?? json.options ?? {} const ctx = json.context - // This guard should not be neccessary -- why would context not exist here? Ditto ^ -- - // page enrichment should add a context to an event by default. - // In any case, adding an empty string 'default'. - // We do not use the current search parameters, as they might be stale by this point. + // This guard against missing ctx.page should not be neccessary, since context.page is always defined const query: string = ctx.page?.search || '' delete json.options From ab86f6339a2bbba22b3dee797e3cb0b82cc43466 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Mon, 17 Apr 2023 16:06:14 -0500 Subject: [PATCH 3/4] add changeset --- .changeset/red-ears-sleep.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .changeset/red-ears-sleep.md diff --git a/.changeset/red-ears-sleep.md b/.changeset/red-ears-sleep.md new file mode 100644 index 000000000..dbf72bf25 --- /dev/null +++ b/.changeset/red-ears-sleep.md @@ -0,0 +1,10 @@ +--- +'@segment/analytics-next': patch +--- + +Fixes a bug where users who override page properties: +```ts +analytics.page(undefined, undefined, {search: "?utm_source=123&utm_content=content" ) +analytics.track("foo", {url: "....", search: "?utm_source=123&utm_content=content" ) +``` +...would get the wrong context.campaign object. From e7e3d7543fa68291711f313da294294da856b12a Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Mon, 17 Apr 2023 16:13:33 -0500 Subject: [PATCH 4/4] add changeset --- .changeset/red-ears-sleep.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.changeset/red-ears-sleep.md b/.changeset/red-ears-sleep.md index dbf72bf25..06b1d2710 100644 --- a/.changeset/red-ears-sleep.md +++ b/.changeset/red-ears-sleep.md @@ -2,9 +2,11 @@ '@segment/analytics-next': patch --- -Fixes a bug where users who override page properties: +Fixes a utm-parameter parsing bug where overridden page.search properties would not be reflected in the context.campaign object ```ts analytics.page(undefined, undefined, {search: "?utm_source=123&utm_content=content" ) analytics.track("foo", {url: "....", search: "?utm_source=123&utm_content=content" ) + +// should result in a context.campaign of: +{ source: 123, content: 'content'} ``` -...would get the wrong context.campaign object.