🐛 [Shopify] Defer plugin init until a checkout page view, fixing double session IDs

packages/browser-core/src/index.ts

@@ -142,6 +142,7 @@ export * from './browser/addEventListener'
 export { requestIdleCallback } from './tools/requestIdleCallback'
 export * from './tools/taskQueue'
 export * from './tools/timer'
+export * from './tools/thenable'
 export type { ConsoleLog } from './domain/console/consoleObservable'
 export { initConsoleObservable } from './domain/console/consoleObservable'
 export { catchUserErrors } from './tools/catchUserErrors'

packages/browser-core/src/tools/thenable.spec.ts

@@ -0,0 +1,42 @@
+import { isThenable, waitForThenable, TIMEOUT_ERROR_MESSAGE } from './thenable'
+import { noop } from './utils/functionUtils'
+
+describe('isThenable', () => {
+  it('returns true for a Promise', () => {
+    expect(isThenable(Promise.resolve())).toBe(true)
+  })
+
+  it('returns true for a plain object with a then function', () => {
+    expect(isThenable({ then: noop })).toBe(true)
+  })
+
+  it('returns false for a plain object without a then function', () => {
+    expect(isThenable({})).toBe(false)
+  })
+
+  it('returns false for null and undefined', () => {
+    expect(isThenable(null)).toBe(false)
+    expect(isThenable(undefined)).toBe(false)
+  })
+
+  it('returns false for primitives', () => {
+    expect(isThenable(42)).toBe(false)
+    expect(isThenable('foo')).toBe(false)
+  })
+})
+
+describe('waitForThenable', () => {
+  it('resolves with the thenable value when it settles before the timeout', async () => {
+    const result = await waitForThenable(Promise.resolve('value'), 1000)
+    expect(result).toBe('value')
+  })
+
+  it('rejects with the thenable rejection reason when it settles before the timeout', async () => {
+    await expectAsync(waitForThenable(Promise.reject(new Error('boom')), 1000)).toBeRejectedWithError('boom')
+  })
+
+  it('rejects with a timeout error when the thenable does not settle in time', async () => {
+    const neverSettles = new Promise(noop)
+    await expectAsync(waitForThenable(neverSettles, 0)).toBeRejectedWithError(TIMEOUT_ERROR_MESSAGE)
+  })
+})

packages/browser-core/src/tools/thenable.ts

@@ -0,0 +1,23 @@
+import type { TimeoutId } from './timer'
+import { setTimeout, clearTimeout } from './timer'
+
+export function isThenable<T>(value: unknown): value is PromiseLike<T> {
+  return !!value && typeof (value as { then?: unknown }).then === 'function'
+}
+
+export const TIMEOUT_ERROR_MESSAGE = 'Timeout'
+export function isTimeoutError(error: unknown): error is Error {
+  return error instanceof Error && error.message === TIMEOUT_ERROR_MESSAGE
+}
+
+/**
+ * Resolves or rejects with `thenable`, or rejects with a `TIMEOUT_ERROR_MESSAGE` error if it
+ * doesn't settle within `timeout` ms.
+ */
+export function waitForThenable<T>(thenable: PromiseLike<T>, timeout = 3000): Promise<T> {
+  let timeoutId: TimeoutId
+  const timeoutPromise = new Promise<never>((_, reject) => {
+    timeoutId = setTimeout(() => reject(new Error(TIMEOUT_ERROR_MESSAGE)), timeout)
+  })
+  return Promise.race([thenable, timeoutPromise]).finally(() => clearTimeout(timeoutId))
+}

packages/browser-rum-angular/src/domain/angularPlugin.spec.ts

@@ -32,6 +32,7 @@ describe('angularPlugin', () => {
 
     expect(callbackSpy).not.toHaveBeenCalled()
 
+    // eslint-disable-next-line @typescript-eslint/no-floating-promises -- onInit never returns a promise for this plugin
     angularPlugin(pluginConfiguration).onInit!({
       publicApi: PUBLIC_API,
       initConfiguration: INIT_CONFIGURATION,
@@ -45,6 +46,7 @@ describe('angularPlugin', () => {
   it('calls callbacks immediately if onInit was already invoked', () => {
     const callbackSpy = jasmine.createSpy()
     const pluginConfiguration = {}
+    // eslint-disable-next-line @typescript-eslint/no-floating-promises -- onInit never returns a promise for this plugin
     angularPlugin(pluginConfiguration).onInit!({
       publicApi: PUBLIC_API,
       initConfiguration: INIT_CONFIGURATION,
@@ -59,13 +61,15 @@ describe('angularPlugin', () => {
 
   it('enforce manual view tracking when router is enabled', () => {
     const initConfiguration = { ...INIT_CONFIGURATION }
+    // eslint-disable-next-line @typescript-eslint/no-floating-promises -- onInit never returns a promise for this plugin
     angularPlugin({ router: true }).onInit!({ publicApi: PUBLIC_API, initConfiguration })
 
     expect(initConfiguration.trackViewsManually).toBe(true)
   })
 
   it('does not enforce manual view tracking when router is disabled', () => {
     const initConfiguration = { ...INIT_CONFIGURATION }
+    // eslint-disable-next-line @typescript-eslint/no-floating-promises -- onInit never returns a promise for this plugin
     angularPlugin({ router: false }).onInit!({ publicApi: PUBLIC_API, initConfiguration })
 
     expect(initConfiguration.trackViewsManually).toBeUndefined()

packages/browser-rum-angular/test/initializeAngularPlugin.ts

@@ -11,6 +11,7 @@ export function initializeAngularPlugin({
   resetAngularPlugin()
   const plugin = angularPlugin()
 
+  // eslint-disable-next-line @typescript-eslint/no-floating-promises -- onInit never returns a promise for this plugin
   plugin.onInit!({
     publicApi: {} as RumPublicApi,
     initConfiguration: {} as RumInitConfiguration,

packages/browser-rum-core/src/boot/preStartRum.spec.ts

@@ -620,6 +620,45 @@ describe('preStartRum', () => {
         expect(doStartRumSpy).toHaveBeenCalled()
         expect(doStartRumSpy.calls.mostRecent().args[0].applicationId).toBe('application-id')
       })
+
+      it('does not start RUM, synchronously, when a plugin synchronously returns false', () => {
+        const plugin: RumPlugin = { name: 'a', onInit: () => false }
+        const { strategy, doStartRumSpy } = createPreStartStrategyWithDefaults()
+
+        strategy.init({ ...DEFAULT_INIT_CONFIGURATION, plugins: [plugin] }, PUBLIC_API)
+
+        expect(doStartRumSpy).not.toHaveBeenCalled()
+      })
+
+      it('starts RUM once a plugin resolves its onInit promise to void', async () => {
+        const plugin: RumPlugin = { name: 'a', onInit: () => Promise.resolve() }
+        const { strategy, doStartRumSpy } = createPreStartStrategyWithDefaults()
+
+        strategy.init({ ...DEFAULT_INIT_CONFIGURATION, plugins: [plugin] }, PUBLIC_API)
+        await collectAsyncCalls(doStartRumSpy, 1)
+
+        expect(doStartRumSpy).toHaveBeenCalled()
+      })
+
+      it('does not start RUM when a plugin resolves its onInit promise to false', async () => {
+        const plugin: RumPlugin = { name: 'a', onInit: () => Promise.resolve(false) }
+        const { strategy, doStartRumSpy } = createPreStartStrategyWithDefaults()
+
+        strategy.init({ ...DEFAULT_INIT_CONFIGURATION, plugins: [plugin] }, PUBLIC_API)
+        await new Promise((resolve) => setTimeout(resolve))
+
+        expect(doStartRumSpy).not.toHaveBeenCalled()
+      })
+
+      it('does not start RUM and does not throw when a plugin onInit promise rejects', async () => {
+        const plugin: RumPlugin = { name: 'a', onInit: () => Promise.reject(new Error('boom')) }
+        const { strategy, doStartRumSpy } = createPreStartStrategyWithDefaults()
+
+        strategy.init({ ...DEFAULT_INIT_CONFIGURATION, plugins: [plugin] }, PUBLIC_API)
+        await new Promise((resolve) => setTimeout(resolve))
+
+        expect(doStartRumSpy).not.toHaveBeenCalled()
+      })
     })
   })
 

packages/browser-rum-core/src/boot/preStartRum.ts

@@ -42,7 +42,7 @@ import {
 } from '../domain/configuration'
 import type { ViewOptions } from '../domain/view/trackViews'
 import type { OperationOptions, FailureReason } from '../domain/vital/vitalCollection'
-import { callPluginsMethod } from '../domain/plugins'
+import { callPluginsOnInit } from '../domain/plugins'
 import { startTrackingConsentContext } from '../domain/contexts/trackingConsentContext'
 import type { StartRumResult } from './startRum'
 import type { RumPublicApiOptions, Strategy } from './rumPublicApi'
@@ -242,31 +242,48 @@ export function createPreStartStrategy(
         return
       }
 
-      callPluginsMethod(initConfiguration.plugins, 'onInit', { initConfiguration, publicApi })
+      const shouldContinue = callPluginsOnInit(initConfiguration.plugins, { initConfiguration, publicApi })
 
-      const hasRemoteConfiguration = getRemoteConfigurationId(initConfiguration)
-
-      if (hasRemoteConfiguration) {
-        const supportedContextManagers = { user: userContext, context: globalContext }
-        const isSyncLoading = !!initConfiguration.remoteConfigurationId || !!initConfiguration.remoteConfiguration?.sync
-
-        if (isSyncLoading) {
-          fetchAndApplyRemoteConfiguration(initConfiguration, supportedContextManagers)
-            .then((resolvedInitConfiguration) => {
-              if (resolvedInitConfiguration) {
-                doInit(resolvedInitConfiguration, errorStack)
-              }
-            })
-            .catch(monitorError)
-        } else {
-          const resolvedInitConfiguration = getRemoteConfiguration(initConfiguration, supportedContextManagers)
+      if (typeof shouldContinue === 'boolean') {
+        if (shouldContinue) {
+          proceedWithInit()
+        }
+      } else {
+        shouldContinue
+          .then((result) => {
+            if (result) {
+              proceedWithInit()
+            }
+          })
+          .catch(monitorError)
+      }
 
-          if (resolvedInitConfiguration) {
-            doInit(resolvedInitConfiguration, errorStack)
+      function proceedWithInit() {
+        const hasRemoteConfiguration = getRemoteConfigurationId(initConfiguration)
+
+        if (hasRemoteConfiguration) {
+          const supportedContextManagers = { user: userContext, context: globalContext }
+          const isSyncLoading =
+            !!initConfiguration.remoteConfigurationId || !!initConfiguration.remoteConfiguration?.sync
+
+          if (isSyncLoading) {
+            fetchAndApplyRemoteConfiguration(initConfiguration, supportedContextManagers)
+              .then((resolvedInitConfiguration) => {
+                if (resolvedInitConfiguration) {
+                  doInit(resolvedInitConfiguration, errorStack)
+                }
+              })
+              .catch(monitorError)
+          } else {
+            const resolvedInitConfiguration = getRemoteConfiguration(initConfiguration, supportedContextManagers)
+
+            if (resolvedInitConfiguration) {
+              doInit(resolvedInitConfiguration, errorStack)
+            }
           }
+        } else {
+          doInit(initConfiguration, errorStack)
         }
-      } else {
-        doInit(initConfiguration, errorStack)
       }
     },
 

packages/browser-rum-core/src/boot/rumPublicApi.ts

@@ -50,7 +50,7 @@ import type {
   FeatureOperationOptions,
   FailureReason,
 } from '../domain/vital/vitalCollection'
-import { callPluginsMethod } from '../domain/plugins'
+import { callPluginsOnRumStart } from '../domain/plugins'
 import type { Hooks } from '../domain/hooks'
 import type { SdkName } from '../domain/contexts/defaultContext'
 import type { ActionOptions } from '../domain/action/trackManualActions'
@@ -714,7 +714,7 @@ export function makeRumPublicApi(
 
       strategy = createPostStartStrategy(strategy, startRumResult)
 
-      callPluginsMethod(configuration.plugins, 'onRumStart', {
+      callPluginsOnRumStart(configuration.plugins, {
         addEvent: startRumResult.addEvent,
         addError: startRumResult.addError,
       })

packages/browser-rum-core/src/domain/plugins.spec.ts

@@ -1,23 +1,122 @@
 import type { RumPublicApi } from '../boot/rumPublicApi'
 import type { RumInitConfiguration } from './configuration'
 import type { RumPlugin } from './plugins'
-import { callPluginsMethod } from './plugins'
+import { callPluginsOnInit, callPluginsOnRumStart } from './plugins'
 
-describe('callPluginsMethod', () => {
-  it('calls the method on each plugin', () => {
+describe('callPluginsOnInit', () => {
+  const PARAMETER = { initConfiguration: {} as RumInitConfiguration, publicApi: {} as RumPublicApi }
+
+  it('calls onInit on each plugin', () => {
     const plugin1 = { name: 'a', onInit: jasmine.createSpy() } satisfies RumPlugin
     const plugin2 = { name: 'b', onInit: jasmine.createSpy() } satisfies RumPlugin
-    const parameter = { initConfiguration: {} as RumInitConfiguration, publicApi: {} as RumPublicApi }
-    callPluginsMethod([plugin1, plugin2], 'onInit', parameter)
-    expect(plugin1.onInit).toHaveBeenCalledWith(parameter)
-    expect(plugin2.onInit).toHaveBeenCalledWith(parameter)
+
+    void callPluginsOnInit([plugin1, plugin2], PARAMETER)
+
+    expect(plugin1.onInit).toHaveBeenCalledWith(PARAMETER)
+    expect(plugin2.onInit).toHaveBeenCalledWith(PARAMETER)
   })
 
-  it('does not call the method if the plugin does not have it', () => {
+  it('does not call onInit if the plugin does not have it', () => {
     const plugin1 = { name: 'a', onInit: jasmine.createSpy() } satisfies RumPlugin
     const plugin2 = { name: 'b' } satisfies RumPlugin
-    const parameter = { initConfiguration: {} as RumInitConfiguration, publicApi: {} as RumPublicApi }
-    callPluginsMethod([plugin1, plugin2], 'onInit', parameter)
-    expect(plugin1.onInit).toHaveBeenCalledWith(parameter)
+
+    expect(() => callPluginsOnInit([plugin1, plugin2], PARAMETER)).not.toThrow()
+    expect(plugin1.onInit).toHaveBeenCalledWith(PARAMETER)
+  })
+
+  it('returns true synchronously when there are no plugins', () => {
+    expect(callPluginsOnInit(undefined, PARAMETER)).toBe(true)
+  })
+
+  it('returns true synchronously when every plugin returns void or true', () => {
+    const plugin1 = { name: 'a', onInit: jasmine.createSpy().and.returnValue(undefined) } satisfies RumPlugin
+    const plugin2 = { name: 'b', onInit: jasmine.createSpy().and.returnValue(true) } satisfies RumPlugin
+
+    const result = callPluginsOnInit([plugin1, plugin2], PARAMETER)
+
+    expect(result).toBe(true)
+    expect(plugin1.onInit).toHaveBeenCalledWith(PARAMETER)
+    expect(plugin2.onInit).toHaveBeenCalledWith(PARAMETER)
+  })
+
+  it('returns false synchronously as soon as a sync plugin returns false, but still calls the other plugins', () => {
+    const plugin1 = { name: 'a', onInit: jasmine.createSpy().and.returnValue(false) } satisfies RumPlugin
+    const plugin2 = { name: 'b', onInit: jasmine.createSpy() } satisfies RumPlugin
+
+    const result = callPluginsOnInit([plugin1, plugin2], PARAMETER)
+
+    expect(result).toBe(false)
+    expect(plugin2.onInit).toHaveBeenCalledWith(PARAMETER)
+  })
+
+  it('returns a Promise once a plugin returns a thenable, and resolves to true if nothing aborts', async () => {
+    const plugin1 = { name: 'a', onInit: () => Promise.resolve() } satisfies RumPlugin
+    const plugin2 = { name: 'b', onInit: jasmine.createSpy().and.returnValue(true) } satisfies RumPlugin
+
+    const result = callPluginsOnInit([plugin1, plugin2], PARAMETER)
+
+    expect(result).not.toBe(true)
+    expect(await result).toBe(true)
+    expect(plugin2.onInit).toHaveBeenCalledWith(PARAMETER)
+  })
+
+  it('resolves to false once an async plugin resolves to false, without waiting for it before calling other plugins', async () => {
+    const plugin1 = { name: 'a', onInit: () => Promise.resolve(false) } satisfies RumPlugin
+    const plugin2 = { name: 'b', onInit: jasmine.createSpy() } satisfies RumPlugin
+
+    const result = callPluginsOnInit([plugin1, plugin2], PARAMETER)
+
+    expect(plugin2.onInit).toHaveBeenCalledWith(PARAMETER)
+    expect(await result).toBe(false)
+  })
+
+  it('does not wait for an earlier plugin to resolve before calling the next plugin', async () => {
+    let clientTokenSeenByPlugin2: string | undefined
+    const plugin1 = {
+      name: 'a',
+      onInit: ({ initConfiguration }: { initConfiguration: RumInitConfiguration }) =>
+        Promise.resolve().then(() => {
+          initConfiguration.clientToken = 'from-async-plugin'
+        }),
+    } satisfies RumPlugin
+    const plugin2 = {
+      name: 'b',
+      onInit: ({ initConfiguration }: { initConfiguration: RumInitConfiguration }) => {
+        clientTokenSeenByPlugin2 = initConfiguration.clientToken
+      },
+    } satisfies RumPlugin
+    const initConfiguration = {} as RumInitConfiguration
+
+    await callPluginsOnInit([plugin1, plugin2], { initConfiguration, publicApi: {} as RumPublicApi })
+
+    expect(clientTokenSeenByPlugin2).toBeUndefined()
+  })
+})
+
+describe('callPluginsOnRumStart', () => {
+  it('calls onRumStart on each plugin', () => {
+    const plugin1 = { name: 'a', onRumStart: jasmine.createSpy() } satisfies RumPlugin
+    const plugin2 = { name: 'b', onRumStart: jasmine.createSpy() } satisfies RumPlugin
+    const options = { addEvent: jasmine.createSpy(), addError: jasmine.createSpy() }
+
+    callPluginsOnRumStart([plugin1, plugin2], options)
+
+    expect(plugin1.onRumStart).toHaveBeenCalledWith(options)
+    expect(plugin2.onRumStart).toHaveBeenCalledWith(options)
+  })
+
+  it('does not call onRumStart if the plugin does not have it', () => {
+    const plugin1 = { name: 'a', onRumStart: jasmine.createSpy() } satisfies RumPlugin
+    const plugin2 = { name: 'b' } satisfies RumPlugin
+    const options = { addEvent: jasmine.createSpy(), addError: jasmine.createSpy() }
+
+    expect(() => callPluginsOnRumStart([plugin1, plugin2], options)).not.toThrow()
+    expect(plugin1.onRumStart).toHaveBeenCalledWith(options)
+  })
+
+  it('does nothing when there are no plugins', () => {
+    expect(() =>
+      callPluginsOnRumStart(undefined, { addEvent: jasmine.createSpy(), addError: jasmine.createSpy() })
+    ).not.toThrow()
   })
 })