Refactor page enrichment plugin (#838)

.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

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)
+  })
+})

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 = <T, K extends keyof T>(
+  object: T,
+  keys: readonly K[]
+): Pick<T, K> =>
+  Object.assign(
+    {},
+    ...keys.map((key) => {
+      if (object && Object.prototype.hasOwnProperty.call(object, key)) {
+        return { [key]: object[key] }
+      }
+    })
+  )

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)
+  })
 })

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,