Make SDK resilient to blocking of non-destination type actions (#1080)

Author: silesky

.changeset/brave-jokes-train.md

@@ -0,0 +1,6 @@
+---
+'@segment/analytics-core': minor
+---
+
+Do not throw errors in .register() method.
+

.changeset/fast-moons-knock.md

@@ -0,0 +1,6 @@
+---
+'@segment/analytics-next': minor
+'@segment/analytics-core': minor
+---
+
+Addresses an issue where, if one of the non-destination actions fails to load/is blocked, the entire SDK fails to load. This is most notable in GA4, where, if GA was blocked, Segment initialization would fail.

packages/browser/src/browser/__tests__/integration.test.ts

@@ -1048,6 +1048,63 @@ describe('timeout', () => {
     expect(analytics.settings.timeout).toEqual(50)
   })
 })
+describe('register', () => {
+  it('will not invoke any plugins that have initialization errors', async () => {
+    const analytics = AnalyticsBrowser.load({
+      writeKey,
+    })
+
+    const errorPlugin = new ActionDestination('Error Plugin', {
+      ...googleAnalytics,
+      load: () => Promise.reject('foo'),
+    })
+    const errorPluginSpy = jest.spyOn(errorPlugin, 'track')
+
+    const goodPlugin = new ActionDestination('Good Plugin', {
+      ...googleAnalytics,
+      load: () => Promise.resolve('foo'),
+    })
+    const goodPluginSpy = jest.spyOn(goodPlugin, 'track')
+
+    await analytics.register(goodPlugin, errorPlugin)
+    await errorPlugin.ready()
+    await goodPlugin.ready()
+
+    await analytics.track('foo')
+
+    expect(errorPluginSpy).not.toHaveBeenCalled()
+    expect(goodPluginSpy).toHaveBeenCalled()
+  })
+
+  it('will emit initialization errors', async () => {
+    const analytics = AnalyticsBrowser.load({
+      writeKey,
+    })
+
+    const errorPlugin = new ActionDestination('Error Plugin', {
+      ...googleAnalytics,
+      load: () => Promise.reject('foo'),
+    })
+
+    const goodPlugin = new ActionDestination('Good Plugin', {
+      ...googleAnalytics,
+      load: () => Promise.resolve('foo'),
+    })
+
+    await analytics
+    const errorPluginName = new Promise<string>((resolve) => {
+      analytics.instance?.queue.on('initialization_failure', (plugin) =>
+        resolve(plugin.name)
+      )
+    })
+
+    await analytics.register(goodPlugin, errorPlugin)
+    await errorPlugin.ready()
+    await goodPlugin.ready()
+
+    expect(await errorPluginName).toBe('Error Plugin')
+  })
+})
 
 describe('deregister', () => {
   beforeEach(async () => {

packages/core/src/plugins/index.ts

@@ -1,11 +1,6 @@
 import type { CoreAnalytics } from '../analytics'
 import type { CoreContext } from '../context'
 
-interface CorePluginConfig {
-  options: any
-  priority: 'critical' | 'non-critical' // whether AJS should expect this plugin to be loaded before starting event delivery
-}
-
 export type PluginType =
   | 'before'
   | 'after'
@@ -26,11 +21,7 @@ export interface CorePlugin<
   version: string
   type: PluginType
   isLoaded: () => boolean
-  load: (
-    ctx: Ctx,
-    instance: Analytics,
-    config?: CorePluginConfig
-  ) => Promise<unknown>
+  load: (ctx: Ctx, instance: Analytics) => Promise<unknown>
 
   unload?: (ctx: Ctx, instance: Analytics) => Promise<unknown> | unknown
   ready?: () => Promise<unknown>

packages/core/src/queue/__tests__/event-queue.test.ts

@@ -611,6 +611,7 @@ describe('Flushing', () => {
 
 describe('register', () => {
   it('only filters out failed destinations after loading', async () => {
+    jest.spyOn(console, 'warn').mockImplementation(noop)
     const eq = new TestEventQueue()
     const goodDestinationPlugin = {
       ...testPlugin,

packages/core/src/queue/__tests__/extension-flushing.test.ts

@@ -53,7 +53,7 @@ describe('Registration', () => {
     expect(load).toHaveBeenCalledWith(ctx, ajs)
   })
 
-  test('fails if plugin cant be loaded', async () => {
+  test('does not reject if a plugin cant be loaded', async () => {
     const eq = new EventQueue()
 
     const plugin: Plugin = {
@@ -65,9 +65,7 @@ describe('Registration', () => {
     }
 
     const ctx = TestCtx.system()
-    await expect(
-      eq.register(ctx, plugin, ajs)
-    ).rejects.toThrowErrorMatchingInlineSnapshot(`"👻"`)
+    await expect(eq.register(ctx, plugin, ajs)).resolves.toBe(undefined)
   })
 
   test('allows for destinations to fail registration', async () => {

packages/core/src/queue/event-queue.ts

@@ -50,25 +50,34 @@ export abstract class CoreEventQueue<
     plugin: Plugin,
     instance: CoreAnalytics
   ): Promise<void> {
-    if (plugin.type === 'destination' && plugin.name !== 'Segment.io') {
-      plugin.load(ctx, instance).catch((err) => {
-        this.failedInitializations.push(plugin.name)
-        this.emit('initialization_failure', plugin)
-        console.warn(plugin.name, err)
-
-        ctx.log('warn', 'Failed to load destination', {
-          plugin: plugin.name,
-          error: err,
-        })
-
-        // Filter out the failed plugin by excluding it from the list
-        this.plugins = this.plugins.filter((p) => p !== plugin)
+    this.plugins.push(plugin)
+
+    const handleLoadError = (err: any) => {
+      this.failedInitializations.push(plugin.name)
+      this.emit('initialization_failure', plugin)
+      console.warn(plugin.name, err)
+
+      ctx.log('warn', 'Failed to load destination', {
+        plugin: plugin.name,
+        error: err,
       })
-    } else {
-      await plugin.load(ctx, instance)
+
+      // Filter out the failed plugin by excluding it from the list
+      this.plugins = this.plugins.filter((p) => p !== plugin)
     }
 
-    this.plugins.push(plugin)
+    if (plugin.type === 'destination' && plugin.name !== 'Segment.io') {
+      plugin.load(ctx, instance).catch(handleLoadError)
+    } else {
+      // for non-destinations plugins, we do need to wait for them to load
+      // reminder: action destinations can require plugins that are not of type "destination".
+      // For example, GA4 loads a type 'before' plugins and addition to a type 'destination' plugin
+      try {
+        await plugin.load(ctx, instance)
+      } catch (err) {
+        handleLoadError(err)
+      }
+    }
   }
 
   async deregister(

packages/node/src/__tests__/integration.test.ts

@@ -406,7 +406,6 @@ describe('version', () => {
 describe('ready', () => {
   it('should only resolve when plugin registration is done', async () => {
     const analytics = createTestAnalytics()
-    expect(analytics['_queue'].plugins.length).toBe(0)
     const result = await analytics.ready
     expect(result).toBeUndefined()
     expect(analytics['_queue'].plugins.length).toBeGreaterThan(0)