Skip to content

Commit f3183f2

Browse files
Fix query string bug (#888)
Co-authored-by: Daniel Jackins <djackins@twilio.com>
1 parent 2689e7d commit f3183f2

File tree

5 files changed

+53
-14
lines changed

5 files changed

+53
-14
lines changed

.changeset/neat-emus-call.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@segment/analytics-next': patch
3+
---
4+
5+
Fix query string parsing bug that was causing events containing the 'search' property with a non string value to be dropped

packages/browser/src/plugins/page-enrichment/__tests__/index.test.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,23 +60,39 @@ describe('Page Enrichment', () => {
6060
test('special page properties in event.properties (url, referrer, etc) are copied to context.page', async () => {
6161
const eventProps = { ...helpers.pageProps }
6262
;(eventProps as any)['should_not_show_up'] = 'hello'
63-
const ctx = await ajs.track('My Event', eventProps)
63+
const ctx = await ajs.page('My Event', eventProps)
6464
const page = ctx.event.context!.page
6565
expect(page).toEqual(
6666
pick(eventProps, ['url', 'path', 'referrer', 'search', 'title'])
6767
)
6868
})
6969

70-
test('event page properties should not be mutated', async () => {
70+
test('special page properties in event.properties (url, referrer, etc) are not copied to context.page in non-page calls', async () => {
7171
const eventProps = { ...helpers.pageProps }
72+
;(eventProps as any)['should_not_show_up'] = 'hello'
7273
const ctx = await ajs.track('My Event', eventProps)
7374
const page = ctx.event.context!.page
75+
expect(page).toMatchInlineSnapshot(`
76+
Object {
77+
"path": "/",
78+
"referrer": "",
79+
"search": "",
80+
"title": "",
81+
"url": "http://localhost/",
82+
}
83+
`)
84+
})
85+
86+
test('event page properties should not be mutated', async () => {
87+
const eventProps = { ...helpers.pageProps }
88+
const ctx = await ajs.page('My Event', eventProps)
89+
const page = ctx.event.context!.page
7490
expect(page).toEqual(eventProps)
7591
})
7692

7793
test('page properties should have defaults', async () => {
7894
const eventProps = pick(helpers.pageProps, ['path', 'referrer'])
79-
const ctx = await ajs.track('My Event', eventProps)
95+
const ctx = await ajs.page('My Event', eventProps)
8096
const page = ctx.event.context!.page
8197
expect(page).toEqual({
8298
...eventProps,
@@ -91,7 +107,7 @@ describe('Page Enrichment', () => {
91107
eventProps.referrer = ''
92108
eventProps.path = undefined as any
93109
eventProps.title = null as any
94-
const ctx = await ajs.track('My Event', eventProps)
110+
const ctx = await ajs.page('My Event', eventProps)
95111
const page = ctx.event.context!.page
96112
expect(page).toEqual(
97113
expect.objectContaining({ referrer: '', path: undefined, title: null })

packages/browser/src/plugins/page-enrichment/index.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { pick } from '../../lib/pick'
22
import type { Context } from '../../core/context'
33
import type { Plugin } from '../../core/plugin'
4+
import { EventProperties } from '@segment/analytics-core'
45

56
interface PageDefault {
67
[key: string]: unknown
@@ -75,23 +76,27 @@ function enrichPageContext(ctx: Context): Context {
7576

7677
const defaultPageContext = pageDefaults()
7778

78-
const pageContextFromEventProps =
79-
event.properties && pick(event.properties, Object.keys(defaultPageContext))
80-
81-
event.context.page = {
82-
...defaultPageContext,
83-
...pageContextFromEventProps,
84-
...event.context.page,
85-
}
79+
let pageContextFromEventProps: Pick<EventProperties, string> | undefined =
80+
undefined
8681

8782
if (event.type === 'page') {
83+
pageContextFromEventProps =
84+
event.properties &&
85+
pick(event.properties, Object.keys(defaultPageContext))
86+
8887
event.properties = {
8988
...defaultPageContext,
9089
...event.properties,
9190
...(event.name ? { name: event.name } : {}),
9291
}
9392
}
9493

94+
event.context.page = {
95+
...defaultPageContext,
96+
...pageContextFromEventProps,
97+
...event.context.page,
98+
}
99+
95100
return ctx
96101
}
97102

packages/browser/src/plugins/segmentio/__tests__/normalize.test.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ describe('before loading', () => {
6969
describe('#normalize', () => {
7070
let object: SegmentEvent
7171
let defaultCtx: any
72-
const withSearchParams = (search?: string) => {
72+
const withSearchParams = (search?: any) => {
7373
object.context = { page: { search } }
7474
}
7575

@@ -299,6 +299,18 @@ describe('before loading', () => {
299299
assert(obj.context.campaign.name === 'overrideName')
300300
})
301301

302+
it('should allow override of .search with object', () => {
303+
withSearchParams({
304+
someObject: 'foo',
305+
})
306+
307+
normalize(analytics, object, options, {})
308+
assert(object)
309+
assert(object.context)
310+
assert(object.context.campaign === undefined)
311+
assert(object.context.referrer === undefined)
312+
})
313+
302314
it('should add .referrer.id and .referrer.type (cookies)', () => {
303315
withSearchParams('?utm_source=source&urid=medium')
304316

packages/browser/src/plugins/segmentio/normalize.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,8 @@ export function normalize(
131131
const ctx = json.context
132132

133133
// This guard against missing ctx.page should not be neccessary, since context.page is always defined
134-
const query: string = ctx.page?.search || ''
134+
const query: string =
135+
typeof ctx.page?.search === 'string' ? ctx.page?.search : ''
135136

136137
delete json.options
137138
json.writeKey = settings?.apiKey

0 commit comments

Comments
 (0)