Skip to content

Commit

Permalink
Capture page context as early as possible if initialPageview = true (
Browse files Browse the repository at this point in the history
  • Loading branch information
silesky authored Jan 3, 2024
1 parent 7b93e7b commit f476038
Show file tree
Hide file tree
Showing 10 changed files with 102 additions and 22 deletions.
5 changes: 5 additions & 0 deletions .changeset/orange-mirrors-sleep.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-next': patch
---

If initialPageview is true, capture page context as early as possible
6 changes: 6 additions & 0 deletions .changeset/plenty-socks-shave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@segment/analytics-consent-tools': minor
'@segment/analytics-consent-wrapper-onetrust': minor
---

If initialPageview is true, call analytics.page() as early as possible to avoid stale page context.
16 changes: 6 additions & 10 deletions packages/browser/src/browser/__tests__/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,21 +300,17 @@ describe('Initialization', () => {
})
})
it('calls page if initialpageview is set', async () => {
jest.mock('../../core/analytics')
const mockPage = jest.fn().mockImplementation(() => Promise.resolve())
Analytics.prototype.page = mockPage

const page = jest.spyOn(Analytics.prototype, 'page')
await AnalyticsBrowser.load({ writeKey }, { initialPageview: true })

expect(mockPage).toHaveBeenCalled()
await sleep(0) // flushed in new task
expect(page).toHaveBeenCalledTimes(1)
})

it('does not call page if initialpageview is not set', async () => {
jest.mock('../../core/analytics')
const mockPage = jest.fn()
Analytics.prototype.page = mockPage
const page = jest.spyOn(Analytics.prototype, 'page')
await AnalyticsBrowser.load({ writeKey }, { initialPageview: false })
expect(mockPage).not.toHaveBeenCalled()
await sleep(0) // flush happens async
expect(page).not.toHaveBeenCalled()
})

it('does not use a persisted queue when disableClientPersistence is true', async () => {
Expand Down
10 changes: 6 additions & 4 deletions packages/browser/src/browser/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
flushAddSourceMiddleware,
flushSetAnonymousID,
flushOn,
PreInitMethodCall,
} from '../core/buffer'
import { ClassicIntegrationSource } from '../plugins/ajs-destination/types'
import { attachInspector } from '../core/inspector'
Expand Down Expand Up @@ -320,6 +321,11 @@ async function loadAnalytics(
// this is an ugly side-effect, but it's for the benefits of the plugins that get their cdn via getCDN()
if (settings.cdnURL) setGlobalCDNUrl(settings.cdnURL)

if (options.initialPageview) {
// capture the page context early, so it's always up-to-date
preInitBuffer.push(new PreInitMethodCall('page', []))
}

let legacySettings =
settings.cdnSettings ??
(await loadLegacySettings(settings.writeKey, settings.cdnURL))
Expand Down Expand Up @@ -374,10 +380,6 @@ async function loadAnalytics(
analytics.initialized = true
analytics.emit('initialize', settings, options)

if (options.initialPageview) {
analytics.page().catch(console.error)
}

await flushFinalBuffer(analytics, preInitBuffer)

return [analytics, ctx]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const mockGetCategories: jest.MockedFn<CreateWrapperSettings['getCategories']> =
jest.fn().mockImplementation(() => ({ Advertising: true }))

const analyticsLoadSpy: jest.MockedFn<AnyAnalytics['load']> = jest.fn()
const analyticsPageSpy: jest.MockedFn<AnyAnalytics['page']> = jest.fn()
const addSourceMiddlewareSpy = jest.fn()
let analyticsOnSpy: jest.MockedFn<AnyAnalytics['on']>
const analyticsTrackSpy: jest.MockedFn<AnyAnalytics['track']> = jest.fn()
Expand Down Expand Up @@ -55,6 +56,7 @@ beforeEach(() => {
})

class MockAnalytics implements AnyAnalytics {
page = analyticsPageSpy
track = analyticsTrackSpy
on = analyticsOnSpy
load = analyticsLoadSpy
Expand Down Expand Up @@ -157,9 +159,7 @@ describe(createWrapper, () => {
}
)

it('should allow segment to be loaded normally (with all consent wrapper behavior disabled) via ctx.abort', async () => {
const mockCdnSettings = cdnSettingsBuilder.build()

it('should pass analytics.load args straight through to the analytics instance if ctx.abort() is called', async () => {
wrapTestAnalytics({
shouldLoadSegment: (ctx) => {
ctx.abort({
Expand All @@ -168,16 +168,21 @@ describe(createWrapper, () => {
},
})

const loadArgs: [any, any] = [
const mockCdnSettings = cdnSettingsBuilder.build()
await analytics.load(
{
...DEFAULT_LOAD_SETTINGS,
cdnSettings: mockCdnSettings,
},
{},
]
await analytics.load(...loadArgs)
{ foo: 'bar' } as any
)
expect(analyticsLoadSpy).toBeCalled()
expect(getAnalyticsLoadLastCall().args).toEqual(loadArgs)
const { args } = getAnalyticsLoadLastCall()
expect(args[0]).toEqual({
...DEFAULT_LOAD_SETTINGS,
cdnSettings: mockCdnSettings,
})
expect(args[1]).toEqual({ foo: 'bar' } as any)
})

it('should allow segment loading to be completely aborted via ctx.abort', async () => {
Expand Down Expand Up @@ -812,4 +817,54 @@ describe(createWrapper, () => {
).toBe(false)
})
})

describe('initialPageview', () => {
it('should send a page event if initialPageview is true', async () => {
wrapTestAnalytics()
await analytics.load(DEFAULT_LOAD_SETTINGS, { initialPageview: true })
expect(analyticsPageSpy).toBeCalledTimes(1)
})

it('should not send a page event if set to false', async () => {
wrapTestAnalytics()
await analytics.load(DEFAULT_LOAD_SETTINGS, { initialPageview: false })
expect(analyticsPageSpy).not.toBeCalled()
})

it('should not be called if set to undefined', async () => {
wrapTestAnalytics()
await analytics.load(DEFAULT_LOAD_SETTINGS, {
initialPageview: undefined,
})
expect(analyticsPageSpy).not.toBeCalled()
})

it('setting should always be set to false when forwarding to the underlying analytics instance', async () => {
wrapTestAnalytics()
await analytics.load(DEFAULT_LOAD_SETTINGS, { initialPageview: true })
const lastCall = getAnalyticsLoadLastCall()
// ensure initialPageview is always set to false so page doesn't get called twice
expect(lastCall.args[1].initialPageview).toBe(false)
})

it('should call page early, even if wrapper is aborted', async () => {
// shouldLoadSegment can take a while to load, so we want to capture page context early so info is not stale
wrapTestAnalytics({
shouldLoadSegment: (ctx) => ctx.abort({ loadSegmentNormally: true }),
})
await analytics.load(DEFAULT_LOAD_SETTINGS, { initialPageview: true })
expect(analyticsPageSpy).toBeCalled()

const lastCall = getAnalyticsLoadLastCall()
// ensure initialPageview is always set to false so analytics.page() doesn't get called twice
expect(lastCall.args[1].initialPageview).toBe(false)
})

it('should buffer a page event even if shouldDisableSegment returns true', async () => {
// in order to capture page info as early as possible
wrapTestAnalytics({ shouldDisableSegment: () => true })
await analytics.load(DEFAULT_LOAD_SETTINGS, { initialPageview: true })
expect(analyticsPageSpy).toBeCalledTimes(1)
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ export class AnalyticsService {
this.analytics.load = loadFn
}

page(): void {
this.analytics.page()
}

configureConsentStampingMiddleware({
getCategories,
pruneUnmappedCategories,
Expand Down
8 changes: 8 additions & 0 deletions packages/consent/consent-tools/src/domain/create-wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ export const createWrapper = <Analytics extends AnyAnalytics>(
settings,
options
): Promise<void> => {
// Prevent stale page context by handling initialPageview ourself.
// By calling page() here early, the current page context (url, etc) gets stored in the pre-init buffer.
// We then set initialPageView to false when we call the underlying analytics library, so page() doesn't get called twice.
if (options?.initialPageview) {
analyticsService.page()
options = { ...options, initialPageview: false }
}

// do not load anything -- segment included
if (await shouldDisableSegment?.()) {
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { AnyAnalytics } from '../../types'

export const analyticsMock: jest.Mocked<AnyAnalytics> = {
addSourceMiddleware: jest.fn(),
page: jest.fn(),
load: jest.fn(),
on: jest.fn(),
track: jest.fn(),
Expand Down
2 changes: 2 additions & 0 deletions packages/consent/consent-tools/src/types/wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export interface AnalyticsBrowserSettings {
export interface InitOptions {
updateCDNSettings?(cdnSettings: CDNSettings): CDNSettings
disable?: boolean | ((cdnSettings: CDNSettings) => boolean)
initialPageview?: boolean
}

/**
Expand All @@ -32,6 +33,7 @@ export interface AnyAnalytics {
addSourceMiddleware(...args: any[]): any
on(event: 'initialize', callback: (cdnSettings: CDNSettings) => void): void
track(event: string, properties?: unknown, ...args: any[]): void
page(): void

/**
* This interface is meant to be compatible with both the snippet (`analytics.load`)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const OneTrustMockGlobal: jest.Mocked<OneTrustGlobal> = {
}

export const analyticsMock: jest.Mocked<AnyAnalytics> = {
page: jest.fn(),
addSourceMiddleware: jest.fn(),
load: jest.fn(),
on: jest.fn(),
Expand Down

0 comments on commit f476038

Please sign in to comment.