Skip to content

Commit

Permalink
Make SDK resilient to blocking of non-destination type actions (#1080)
Browse files Browse the repository at this point in the history
  • Loading branch information
silesky authored Apr 29, 2024
1 parent 77e3cf9 commit e884b61
Show file tree
Hide file tree
Showing 8 changed files with 98 additions and 31 deletions.
6 changes: 6 additions & 0 deletions .changeset/brave-jokes-train.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@segment/analytics-core': minor
---

Do not throw errors in .register() method.

6 changes: 6 additions & 0 deletions .changeset/fast-moons-knock.md
Original file line number Diff line number Diff line change
@@ -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.
57 changes: 57 additions & 0 deletions packages/browser/src/browser/__tests__/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
11 changes: 1 addition & 10 deletions packages/core/src/plugins/index.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand All @@ -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>
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/queue/__tests__/event-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 2 additions & 4 deletions packages/core/src/queue/__tests__/extension-flushing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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 () => {
Expand Down
41 changes: 25 additions & 16 deletions packages/core/src/queue/event-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
1 change: 0 additions & 1 deletion packages/node/src/__tests__/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit e884b61

Please sign in to comment.