From 55a48a0d1e3589fc6b4896e94b79c857cabf1006 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Mon, 17 Apr 2023 14:54:45 -0500 Subject: [PATCH] Refactor page enrichment plugin (#838) --- .changeset/quiet-islands-kneel.md | 5 + .../browser/src/lib/__tests__/pick.test.ts | 34 ++++++ packages/browser/src/lib/pick.ts | 17 +++ .../page-enrichment/__tests__/index.test.ts | 109 +++++++++++++++--- .../src/plugins/page-enrichment/index.ts | 64 ++++------ 5 files changed, 170 insertions(+), 59 deletions(-) create mode 100644 .changeset/quiet-islands-kneel.md create mode 100644 packages/browser/src/lib/__tests__/pick.test.ts create mode 100644 packages/browser/src/lib/pick.ts diff --git a/.changeset/quiet-islands-kneel.md b/.changeset/quiet-islands-kneel.md new file mode 100644 index 000000000..a150cb352 --- /dev/null +++ b/.changeset/quiet-islands-kneel.md @@ -0,0 +1,5 @@ +--- +'@segment/analytics-next': patch +--- + +Refactor page enrichment to only call page defaults once, and simplify logic diff --git a/packages/browser/src/lib/__tests__/pick.test.ts b/packages/browser/src/lib/__tests__/pick.test.ts new file mode 100644 index 000000000..103dd5c27 --- /dev/null +++ b/packages/browser/src/lib/__tests__/pick.test.ts @@ -0,0 +1,34 @@ +import { pick } from '../pick' + +describe(pick, () => { + it.each([ + { + obj: { a: 1, b: '2', c: 3 }, + keys: ['a', 'c'], + expected: { a: 1, c: 3 }, + }, + { + obj: { a: 1, '0': false, c: 3 }, + keys: ['a', '0'], + expected: { a: 1, '0': false }, + }, + { obj: { a: 1, b: '2', c: 3 }, keys: [], expected: {} }, + { + obj: { a: undefined, b: null, c: 1 }, + keys: ['a', 'b'], + expected: { a: undefined, b: null }, + }, + ])('.pick($obj, $keys)', ({ obj, keys, expected }) => { + expect(pick(obj, keys as (keyof typeof obj)[])).toEqual(expected) + }) + it('does not mutate object reference', () => { + const e = { + obj: { a: 1, '0': false, c: 3 }, + keys: ['a', '0'] as const, + expected: { a: 1, '0': false }, + } + const ogObj = { ...e.obj } + pick(e.obj, e.keys) + expect(e.obj).toEqual(ogObj) + }) +}) diff --git a/packages/browser/src/lib/pick.ts b/packages/browser/src/lib/pick.ts new file mode 100644 index 000000000..e2c327dba --- /dev/null +++ b/packages/browser/src/lib/pick.ts @@ -0,0 +1,17 @@ +/** + * @example + * pick({ 'a': 1, 'b': '2', 'c': 3 }, ['a', 'c']) + * => { 'a': 1, 'c': 3 } + */ +export const pick = ( + object: T, + keys: readonly K[] +): Pick => + Object.assign( + {}, + ...keys.map((key) => { + if (object && Object.prototype.hasOwnProperty.call(object, key)) { + return { [key]: object[key] } + } + }) + ) diff --git a/packages/browser/src/plugins/page-enrichment/__tests__/index.test.ts b/packages/browser/src/plugins/page-enrichment/__tests__/index.test.ts index 9b66e607c..584f61243 100644 --- a/packages/browser/src/plugins/page-enrichment/__tests__/index.test.ts +++ b/packages/browser/src/plugins/page-enrichment/__tests__/index.test.ts @@ -1,8 +1,21 @@ import { Analytics } from '../../../core/analytics' import { pageEnrichment, pageDefaults } from '..' +import { pick } from '../../../lib/pick' let ajs: Analytics +const helpers = { + get pageProps() { + return { + url: 'http://foo.com/bar?foo=hello_world', + path: '/bar', + search: '?foo=hello_world', + referrer: 'http://google.com', + title: 'Hello World', + } + }, +} + describe('Page Enrichment', () => { beforeEach(async () => { ajs = new Analytics({ @@ -43,6 +56,49 @@ describe('Page Enrichment', () => { `) }) + describe('event.properties override behavior', () => { + test('special page properties in event.properties (url, referrer, etc) are copied to context.page', async () => { + const eventProps = { ...helpers.pageProps } + ;(eventProps as any)['should_not_show_up'] = 'hello' + const ctx = await ajs.track('My Event', eventProps) + const page = ctx.event.context!.page + expect(page).toEqual( + pick(eventProps, ['url', 'path', 'referrer', 'search', 'title']) + ) + }) + + test('event page properties should not be mutated', async () => { + const eventProps = { ...helpers.pageProps } + const ctx = await ajs.track('My Event', eventProps) + const page = ctx.event.context!.page + expect(page).toEqual(eventProps) + }) + + test('page properties should have defaults', async () => { + const eventProps = pick(helpers.pageProps, ['path', 'referrer']) + const ctx = await ajs.track('My Event', eventProps) + const page = ctx.event.context!.page + expect(page).toEqual({ + ...eventProps, + url: 'http://localhost/', + search: '', + title: '', + }) + }) + + test('undefined / null / empty string properties on event get overridden as usual', async () => { + const eventProps = { ...helpers.pageProps } + eventProps.referrer = '' + eventProps.path = undefined as any + eventProps.title = null as any + const ctx = await ajs.track('My Event', eventProps) + const page = ctx.event.context!.page + expect(page).toEqual( + expect.objectContaining({ referrer: '', path: undefined, title: null }) + ) + }) + }) + test('enriches page events with the page context', async () => { const ctx = await ajs.page( 'My event', @@ -51,28 +107,41 @@ describe('Page Enrichment', () => { ) expect(ctx.event.context?.page).toMatchInlineSnapshot(` - Object { - "path": "/", - "referrer": "", - "search": "", - "title": "", - "url": "not-localhost", - } - `) + Object { + "path": "/", + "referrer": "", + "search": "", + "title": "", + "url": "not-localhost", + } + `) }) - test('enriches page events using properties', async () => { const ctx = await ajs.page('My event', { banana: 'phone', referrer: 'foo' }) expect(ctx.event.context?.page).toMatchInlineSnapshot(` - Object { - "path": "/", - "referrer": "foo", - "search": "", - "title": "", - "url": "http://localhost/", - } - `) + Object { + "path": "/", + "referrer": "foo", + "search": "", + "title": "", + "url": "http://localhost/", + } + `) + }) + + test('in page events, event.name overrides event.properties.name', async () => { + const ctx = await ajs.page('My Event', undefined, undefined, { + name: 'some propery name', + }) + expect(ctx.event.properties!.name).toBe('My Event') + }) + + test('in non-page events, event.name does not override event.properties.name', async () => { + const ctx = await ajs.track('My Event', { + name: 'some propery name', + }) + expect(ctx.event.properties!.name).toBe('some propery name') }) test('enriches identify events with the page context', async () => { @@ -147,4 +216,10 @@ describe('pageDefaults', () => { const defs = pageDefaults() expect(defs.url).toEqual('http://www.segment.local?test=true') }) + + it('if canonical does not exist, returns fallback', () => { + document.body.appendChild(el) + const defs = pageDefaults() + expect(defs.url).toEqual(window.location.href) + }) }) diff --git a/packages/browser/src/plugins/page-enrichment/index.ts b/packages/browser/src/plugins/page-enrichment/index.ts index 3b0be18aa..6a1410057 100644 --- a/packages/browser/src/plugins/page-enrichment/index.ts +++ b/packages/browser/src/plugins/page-enrichment/index.ts @@ -1,3 +1,4 @@ +import { pick } from '../../lib/pick' import type { Context } from '../../core/context' import type { Plugin } from '../../core/plugin' @@ -12,20 +13,12 @@ interface PageDefault { /** * Get the current page's canonical URL. - * - * @return {string|undefined} */ -function canonical(): string { - const tags = document.getElementsByTagName('link') - let canon: string | null = '' - - Array.prototype.slice.call(tags).forEach((tag) => { - if (tag.getAttribute('rel') === 'canonical') { - canon = tag.getAttribute('href') - } - }) - - return canon +function canonical(): string | undefined { + const canon = document.querySelector("link[rel='canonical']") + if (canon) { + return canon.getAttribute('href') || undefined + } } /** @@ -79,24 +72,25 @@ export function pageDefaults(): PageDefault { function enrichPageContext(ctx: Context): Context { const event = ctx.event event.context = event.context || {} - let pageContext = pageDefaults() - const pageProps = event.properties ?? {} - Object.keys(pageContext).forEach((key) => { - if (pageProps[key]) { - pageContext[key] = pageProps[key] - } - }) + const defaultPageContext = pageDefaults() - if (event.context.page) { - pageContext = Object.assign({}, pageContext, event.context.page) - } + const pageContextFromEventProps = + event.properties && pick(event.properties, Object.keys(defaultPageContext)) - event.context = Object.assign({}, event.context, { - page: pageContext, - }) + event.context.page = { + ...defaultPageContext, + ...pageContextFromEventProps, + ...event.context.page, + } - ctx.event = event + if (event.type === 'page') { + event.properties = { + ...defaultPageContext, + ...event.properties, + ...(event.name ? { name: event.name } : {}), + } + } return ctx } @@ -107,21 +101,7 @@ export const pageEnrichment: Plugin = { isLoaded: () => true, load: () => Promise.resolve(), type: 'before', - - page: (ctx) => { - ctx.event.properties = Object.assign( - {}, - pageDefaults(), - ctx.event.properties - ) - - if (ctx.event.name) { - ctx.event.properties.name = ctx.event.name - } - - return enrichPageContext(ctx) - }, - + page: enrichPageContext, alias: enrichPageContext, track: enrichPageContext, identify: enrichPageContext,