From e35a7150a4f4cf32a6d69c8b81bc540a14a47d34 Mon Sep 17 00:00:00 2001 From: Oscar Bazaldua <511911+oscb@users.noreply.github.com> Date: Tue, 9 Nov 2021 14:05:33 -0800 Subject: [PATCH 1/3] fix: custom event integrations override support, refactor integration merge logic --- example/e2e/main.e2e.js | 2 +- packages/core/src/__tests__/api.test.ts | 12 +- packages/core/src/analytics.ts | 98 +++++++------- packages/core/src/api.ts | 18 +-- packages/core/src/client.tsx | 2 +- packages/core/src/events.ts | 4 +- packages/core/src/plugin.ts | 12 -- .../core/src/plugins/SegmentDestination.ts | 54 ++++---- .../__tests__/SegmentDestination.test.ts | 123 ++++++++++++++++++ packages/core/src/store/system.ts | 46 ++++--- packages/core/src/timeline.ts | 2 +- packages/core/src/types.ts | 46 +++---- 12 files changed, 263 insertions(+), 156 deletions(-) create mode 100644 packages/core/src/plugins/__tests__/SegmentDestination.test.ts diff --git a/example/e2e/main.e2e.js b/example/e2e/main.e2e.js index 2d12e47d0..44cc5fb37 100644 --- a/example/e2e/main.e2e.js +++ b/example/e2e/main.e2e.js @@ -142,7 +142,7 @@ describe('#mainTest', () => { expect(mockServerListener).toHaveBeenCalledTimes(1); const request = mockServerListener.mock.calls[0][0]; - const context = request.context; + const context = request.batch[0].context; expect(request.batch).toHaveLength(1); expect(context.app.name).toBe('Analytics'); diff --git a/packages/core/src/__tests__/api.test.ts b/packages/core/src/__tests__/api.test.ts index 0290fb27d..74fbe1cc7 100644 --- a/packages/core/src/__tests__/api.test.ts +++ b/packages/core/src/__tests__/api.test.ts @@ -1,7 +1,7 @@ import { Context, EventType, - Integrations, + SegmentAPIIntegrations, TrackEventType, UserTraits, } from '../types'; @@ -43,7 +43,7 @@ describe('#sendEvents', () => { // Context and Integration exist on SegmentEvents but are transmitted separately to avoid duplication const additionalEventProperties: { context: Context; - integrations: Integrations; + integrations: SegmentAPIIntegrations; } = { context: await context.getContext({ name: 'Hello' }), integrations: { @@ -66,13 +66,7 @@ describe('#sendEvents', () => { expect(fetch).toHaveBeenCalledWith('https://api.segment.io/v1/batch', { method: 'POST', body: JSON.stringify({ - batch: [ - { ...serializedEventProperties, sentAt: '2001-01-01T00:00:00.000Z' }, - ], - context: { appName: 'Segment Example', traits: { name: 'Hello' } }, - integrations: { - Firebase: false, - }, + batch: [{ ...event, sentAt: '2001-01-01T00:00:00.000Z' }], }), headers: { 'Authorization': 'Basic U0VHTUVOVF9LRVk6', diff --git a/packages/core/src/analytics.ts b/packages/core/src/analytics.ts index c40b7bb44..343f7f1d1 100644 --- a/packages/core/src/analytics.ts +++ b/packages/core/src/analytics.ts @@ -26,10 +26,11 @@ import { Store, } from './store'; import { Timeline } from './timeline'; -import type { +import { Config, GroupTraits, JsonMap, + PluginType, SegmentAPISettings, SegmentEvent, UserTraits, @@ -106,13 +107,34 @@ export class SegmentClient { return plugins; } - settings() { - let settings: SegmentAPISettings | undefined; + /** + * Retrieves a copy of the settings + * @returns Configuration object for all plugins + */ + getSettings() { const { system } = this.store.getState(); - if (system.settings) { - settings = system.settings; + return { integrations: { ...system.settings?.integrations } }; + } + + /** + * Returns the plugins currently loaded in the timeline + * @param ofType Type of plugins, defaults to all + * @returns List of Plugin objects + */ + getPlugins(ofType?: PluginType): readonly Plugin[] { + const plugins = { ...this.timeline.plugins }; + if (ofType !== undefined) { + return [...(plugins[ofType] ?? [])]; } - return settings; + return ( + [ + ...this.getPlugins(PluginType.before), + ...this.getPlugins(PluginType.enrichment), + ...this.getPlugins(PluginType.utility), + ...this.getPlugins(PluginType.destination), + ...this.getPlugins(PluginType.after), + ] ?? [] + ); } constructor({ @@ -159,7 +181,7 @@ export class SegmentClient { ); } - async getSettings() { + async fetchSettings() { await getSettings.bind(this)(); } @@ -270,29 +292,34 @@ export class SegmentClient { } /** - Adds a new plugin to the currently loaded set. - - - Parameter plugin: The plugin to be added. - - Returns: Returns the name of the supplied plugin. - - */ - add({ plugin }: { plugin: Plugin }) { + * Adds a new plugin to the currently loaded set. + * @param {{ plugin: Plugin, settings?: SegmentAPISettings }} Plugin to be added. Settings are optional if you want to force a configuration instead of the Segment Cloud received one + */ + add({ + plugin, + settings, + }: { + plugin: Plugin; + settings?: Plugin extends DestinationPlugin ? SegmentAPISettings : never; + }) { // plugins can either be added immediately or // can be cached and added later during the next state update // this is to avoid adding plugins before network requests made as part of setup have resolved + if (settings !== undefined && plugin.type === PluginType.destination) { + this.store.dispatch( + this.actions.system.addDestination({ + destination: { + key: (plugin as DestinationPlugin).key, + settings, + }, + }) + ); + } + if (!this.isReady) { this.pluginsToAdd.push(plugin); } else { this.addPlugin(plugin); - const isIntegration = this.isNonSegmentDestinationPlugin(plugin); - if (isIntegration) { - // need to maintain the list of integrations to inject into payload - this.store.dispatch( - this.actions.system.addIntegrations([ - { key: (plugin as DestinationPlugin).key }, - ]) - ); - } } } @@ -320,14 +347,6 @@ export class SegmentClient { */ remove({ plugin }: { plugin: Plugin }) { this.timeline.remove(plugin); - const isIntegration = this.isNonSegmentDestinationPlugin(plugin); - if (isIntegration) { - this.store.dispatch( - this.actions.system.removeIntegration({ - key: (plugin as DestinationPlugin).key, - }) - ); - } } process(incomingEvent: SegmentEvent) { @@ -348,27 +367,10 @@ export class SegmentClient { this.addPlugin(plugin); }); - // filter to see if we need to register any - const destPlugins = this.pluginsToAdd.filter( - this.isNonSegmentDestinationPlugin - ); - // now that they're all added, clear the cache // this prevents this block running for every update this.pluginsToAdd = []; - // if we do have destPlugins, bulk-register them with the system - // this isn't done as part of addPlugin to avoid dispatching an update as part of an update - // which can lead to an infinite loop - // this is safe to fire & forget here as we've cleared pluginsToAdd - if (destPlugins.length > 0) { - this.store.dispatch( - this.actions.system.addIntegrations( - (destPlugins as DestinationPlugin[]).map(({ key }) => ({ key })) - ) - ); - } - // finally set the flag which means plugins will be added + registered immediately in future this.isReady = true; } finally { diff --git a/packages/core/src/api.ts b/packages/core/src/api.ts index 3cf0cd645..7c20a4173 100644 --- a/packages/core/src/api.ts +++ b/packages/core/src/api.ts @@ -9,25 +9,15 @@ export const sendEvents = async ({ config: Config; events: SegmentEvent[]; }) => { - const updatedEvents = events.map((event) => { - const updatedEvent = { - ...event, - sentAt: new Date().toISOString(), - }; - - // Context and Integration exist on SegmentEvents but are transmitted separately to avoid duplication - delete updatedEvent.context; - delete updatedEvent.integrations; - - return updatedEvent; - }); + const updatedEvents = events.map((event) => ({ + ...event, + sentAt: new Date().toISOString(), + })); await fetch(batchApi, { method: 'POST', body: JSON.stringify({ batch: updatedEvents, - context: events[0].context, - integrations: events[0].integrations, }), headers: { 'Authorization': `Basic ${Base64.encode(`${config.writeKey}:`)}`, diff --git a/packages/core/src/client.tsx b/packages/core/src/client.tsx index 8e06224ac..03bda0315 100644 --- a/packages/core/src/client.tsx +++ b/packages/core/src/client.tsx @@ -13,7 +13,7 @@ const doClientSetup = async (client: SegmentClient) => { await client.bootstrapStore(); // get destination settings - await client.getSettings(); + await client.fetchSettings(); // flush any stored events client.flush(); diff --git a/packages/core/src/events.ts b/packages/core/src/events.ts index f053d9ad8..6f17f8883 100644 --- a/packages/core/src/events.ts +++ b/packages/core/src/events.ts @@ -79,14 +79,14 @@ const isAliasEvent = (event: SegmentEvent): event is AliasEventType => event.type === EventType.AliasEvent; export const applyRawEventData = (event: SegmentEvent, store: Store) => { - const { system, userInfo } = store.getState(); + const { userInfo } = store.getState(); return { ...event, anonymousId: userInfo.anonymousId, messageId: getUUID(), timestamp: new Date().toISOString(), - integrations: system.integrations, + integrations: event.integrations ?? {}, userId: isAliasEvent(event) ? event.userId : userInfo.userId, }; }; diff --git a/packages/core/src/plugin.ts b/packages/core/src/plugin.ts index 44b5d4623..d1ce4358c 100644 --- a/packages/core/src/plugin.ts +++ b/packages/core/src/plugin.ts @@ -24,7 +24,6 @@ export class Plugin { this.analytics = analytics; } - // @ts-ignore update(settings: SegmentAPISettings, type: UpdateType) { // do nothing by default, user can override. } @@ -138,17 +137,6 @@ export class DestinationPlugin extends EventPlugin { */ remove(plugin: Plugin) { this.timeline.remove(plugin); - const isSegmentDestination = - Object.getPrototypeOf(plugin).constructor.name === 'SegmentDestination'; - if (!isSegmentDestination) { - const destPlugin = plugin as DestinationPlugin; - if (destPlugin.key) { - const { store, actions } = this.analytics!; - store.dispatch( - actions.system.addIntegrations([{ key: destPlugin.key }]) - ); - } - } } // find(pluginType: PluginType) { diff --git a/packages/core/src/plugins/SegmentDestination.ts b/packages/core/src/plugins/SegmentDestination.ts index 6ba0ee32c..581102331 100644 --- a/packages/core/src/plugins/SegmentDestination.ts +++ b/packages/core/src/plugins/SegmentDestination.ts @@ -1,17 +1,12 @@ import { DestinationPlugin } from '../plugin'; import { + IntegrationSettings, PluginType, + SegmentAPIIntegrations, SegmentAPISettings, SegmentEvent, UpdateType, } from '../types'; -import type { - AliasEventType, - GroupEventType, - IdentifyEventType, - ScreenEventType, - TrackEventType, -} from '../types'; import { chunk } from '../util'; import { sendEvents } from '../api'; @@ -26,29 +21,32 @@ export class SegmentDestination extends DestinationPlugin { // see flush() below } - identify(event: IdentifyEventType) { - this.queueEvent(event); - return event; - } - - track(event: TrackEventType) { - this.queueEvent(event); - return event; - } - - screen(event: ScreenEventType) { - this.queueEvent(event); - return event; - } + execute(event: SegmentEvent): SegmentEvent { + const pluginSettings = this.analytics?.getSettings(); + const plugins = this.analytics?.getPlugins(PluginType.destination); - alias(event: AliasEventType) { - this.queueEvent(event); - return event; - } + // Disable all destinations that have a device mode plugin + const deviceModePlugins = + plugins?.map((plugin) => (plugin as DestinationPlugin).key) ?? []; + const cloudSettings: SegmentAPIIntegrations = { + ...pluginSettings?.integrations, + }; + for (const key of deviceModePlugins) { + if (key in cloudSettings) { + cloudSettings[key] = false; + } + } - group(event: GroupEventType) { - this.queueEvent(event); - return event; + // User/event defined integrations override the cloud/device mode merge + const mergedEvent = { + ...event, + integrations: { + ...cloudSettings, + ...event?.integrations, + }, + }; + this.queueEvent(mergedEvent); + return mergedEvent; } queueEvent(event: SegmentEvent) { diff --git a/packages/core/src/plugins/__tests__/SegmentDestination.test.ts b/packages/core/src/plugins/__tests__/SegmentDestination.test.ts new file mode 100644 index 000000000..f03eb2ba4 --- /dev/null +++ b/packages/core/src/plugins/__tests__/SegmentDestination.test.ts @@ -0,0 +1,123 @@ +import { EventType, TrackEventType } from '../../types'; +import { SegmentDestination } from '../SegmentDestination'; +import { SegmentClient } from '../../analytics'; + +jest.mock('../../analytics', () => ({ + SegmentClient: jest.fn().mockImplementation(() => { + return { + getSettings: jest.fn(), + getPlugins: jest.fn(), + store: { + dispatch: jest.fn(), + }, + actions: { + main: { + addEvent: jest.fn(), + }, + }, + }; + }), +})); + +describe('SegmentDestination', () => { + beforeEach(() => { + (SegmentClient as jest.Mock).mockClear(); + }); + + it('executes', () => { + const plugin = new SegmentDestination(); + // @ts-ignore + plugin.analytics = new SegmentClient(); + const event: TrackEventType = { + anonymousId: '3534a492-e975-4efa-a18b-3c70c562fec2', + event: 'Awesome event', + type: EventType.TrackEvent, + properties: {}, + timestamp: '2000-01-01T00:00:00.000Z', + messageId: '1d1744bf-5beb-41ac-ad7a-943eac33babc', + context: { app: { name: 'TestApp' } }, + integrations: { + Firebase: false, + }, + }; + const result = plugin.execute(event); + expect(result).toEqual(event); + }); + + it('disables device mode plugins to prevent dups', () => { + const plugin = new SegmentDestination(); + // @ts-ignore + plugin.analytics = new SegmentClient(); + plugin.analytics.getSettings = jest.fn().mockReturnValue({ + integrations: { + firebase: { + someConfig: 'someValue', + }, + }, + }); + + plugin.analytics.getPlugins = jest.fn().mockReturnValue([ + { + key: 'firebase', + type: 'destination', + }, + ]); + + const event: TrackEventType = { + anonymousId: '3534a492-e975-4efa-a18b-3c70c562fec2', + event: 'Awesome event', + type: EventType.TrackEvent, + properties: {}, + timestamp: '2000-01-01T00:00:00.000Z', + messageId: '1d1744bf-5beb-41ac-ad7a-943eac33babc', + context: { app: { name: 'TestApp' } }, + integrations: {}, + }; + + const expectedIntegrations = { + firebase: false, + }; + + const result = plugin.execute(event); + expect(result).toEqual({ + ...event, + integrations: expectedIntegrations, + }); + }); + + it('lets plugins/events override destination settings', () => { + const plugin = new SegmentDestination(); + // @ts-ignore + plugin.analytics = new SegmentClient(); + plugin.analytics.getSettings = jest.fn().mockReturnValue({ + integrations: { + firebase: { + someConfig: 'someValue', + }, + }, + }); + + plugin.analytics.getPlugins = jest.fn().mockReturnValue([ + { + key: 'firebase', + type: 'destination', + }, + ]); + + const event: TrackEventType = { + anonymousId: '3534a492-e975-4efa-a18b-3c70c562fec2', + event: 'Awesome event', + type: EventType.TrackEvent, + properties: {}, + timestamp: '2000-01-01T00:00:00.000Z', + messageId: '1d1744bf-5beb-41ac-ad7a-943eac33babc', + context: { app: { name: 'TestApp' } }, + integrations: { + firebase: true, + }, + }; + + const result = plugin.execute(event); + expect(result).toEqual(event); + }); +}); diff --git a/packages/core/src/store/system.ts b/packages/core/src/store/system.ts index ee702fed1..f1e60b652 100644 --- a/packages/core/src/store/system.ts +++ b/packages/core/src/store/system.ts @@ -1,10 +1,9 @@ import { createSlice, PayloadAction } from '@reduxjs/toolkit'; import { defaultConfig } from '../constants'; -import type { SegmentAPISettings, Config, Integrations } from '../types'; +import type { SegmentAPISettings, Config, IntegrationSettings } from '../types'; type SystemState = { configuration: Config; - integrations?: Integrations; settings?: SegmentAPISettings; }; @@ -23,23 +22,34 @@ export default createSlice({ state, action: PayloadAction<{ settings: SegmentAPISettings }> ) => { - state.settings = action.payload.settings; + return { + ...state, + settings: { + ...state.settings, + ...action.payload.settings, + }, + }; }, - addIntegrations: (state, action: PayloadAction<{ key: string }[]>) => { - // we need to set any destination plugins to false in the - // integrations payload. this prevents them from being sent - // by segment.com once an event reaches segment. - if (!state.integrations) { - state.integrations = {}; - } - action.payload.forEach(({ key }) => { - state.integrations![key] = false; - }); - }, - removeIntegration: (state, action: PayloadAction<{ key: string }>) => { - if (state.integrations) { - delete state.integrations[action.payload.key]; - } + addDestination: ( + state, + action: PayloadAction<{ + destination: { + key: string; + settings: IntegrationSettings; + }; + }> + ) => { + return { + ...state, + settings: { + ...state.settings, + integrations: { + ...state.settings?.integrations, + [action.payload.destination.key]: + action.payload.destination.settings, + }, + }, + }; }, }, }); diff --git a/packages/core/src/timeline.ts b/packages/core/src/timeline.ts index 8a0c73d9c..d0a2e7111 100644 --- a/packages/core/src/timeline.ts +++ b/packages/core/src/timeline.ts @@ -25,7 +25,7 @@ export class Timeline { } else { this.plugins[type] = [plugin]; } - const settings = plugin.analytics?.settings(); + const settings = plugin.analytics?.getSettings(); if (settings) { plugin.update(settings, UpdateType.initial); } diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index f74290ea0..649b3031a 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -19,48 +19,44 @@ export type SegmentEvent = | GroupEventType | AliasEventType; -export type Integrations = { - [key: string]: false | SegmentAmplitudeIntegration; -}; - -type BaseEventType = { +interface BaseEventType { anonymousId?: string; messageId?: string; userId?: string; timestamp?: string; context?: PartialContext; - integrations?: Integrations; -}; + integrations?: SegmentAPIIntegrations; +} -export type TrackEventType = BaseEventType & { +export interface TrackEventType extends BaseEventType { type: EventType.TrackEvent; event: string; properties?: JsonMap; -}; +} -export type ScreenEventType = BaseEventType & { +export interface ScreenEventType extends BaseEventType { type: EventType.ScreenEvent; name: string; properties: JsonMap; -}; +} -export type IdentifyEventType = BaseEventType & { +export interface IdentifyEventType extends BaseEventType { type: EventType.IdentifyEvent; traits: UserTraits; -}; +} -export type GroupEventType = BaseEventType & { +export interface GroupEventType extends BaseEventType { type: EventType.GroupEvent; groupId: string; traits: GroupTraits; -}; +} -export type AliasEventType = BaseEventType & { +export interface AliasEventType extends BaseEventType { type: EventType.AliasEvent; userId?: string; previousId: string; -}; +} export type UserTraits = JsonMap & { address?: { @@ -243,12 +239,18 @@ export type SegmentAdjustSettings = { delayTime?: number; }; +export type IntegrationSettings = + // Strongly typed known integration settings + | SegmentAPIIntegration + | SegmentAmplitudeIntegration + | SegmentAdjustSettings + // Support any kind of configuration in the future + | Record + // enable/disable the integration at cloud level + | boolean; + export type SegmentAPIIntegrations = { - [key: string]: - | SegmentAPIIntegration - | SegmentAmplitudeIntegration - | SegmentAdjustSettings - | false; + [key: string]: IntegrationSettings; }; export type SegmentAPISettings = { From c3be1618b9d2228ab417fe35834c93588344fae6 Mon Sep 17 00:00:00 2001 From: Oscar Bazaldua <511911+oscb@users.noreply.github.com> Date: Tue, 9 Nov 2021 14:18:51 -0800 Subject: [PATCH 2/3] fix: Removed type --- packages/core/src/plugins/SegmentDestination.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/src/plugins/SegmentDestination.ts b/packages/core/src/plugins/SegmentDestination.ts index 581102331..7c785754e 100644 --- a/packages/core/src/plugins/SegmentDestination.ts +++ b/packages/core/src/plugins/SegmentDestination.ts @@ -1,6 +1,5 @@ import { DestinationPlugin } from '../plugin'; import { - IntegrationSettings, PluginType, SegmentAPIIntegrations, SegmentAPISettings, From e2cc5f1f480890492a8ae664cc78517359af3827 Mon Sep 17 00:00:00 2001 From: Oscar Bazaldua <511911+oscb@users.noreply.github.com> Date: Tue, 9 Nov 2021 14:23:05 -0800 Subject: [PATCH 3/3] fix: TS errors --- packages/core/src/analytics.ts | 12 ------------ packages/core/src/plugin.ts | 1 + 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/core/src/analytics.ts b/packages/core/src/analytics.ts index 343f7f1d1..737c2d1cc 100644 --- a/packages/core/src/analytics.ts +++ b/packages/core/src/analytics.ts @@ -328,18 +328,6 @@ export class SegmentClient { this.timeline.add(plugin); } - private isNonSegmentDestinationPlugin(plugin: Plugin) { - const isSegmentDestination = - Object.getPrototypeOf(plugin).constructor.name === 'SegmentDestination'; - if (!isSegmentDestination) { - const destPlugin = plugin as DestinationPlugin; - if (destPlugin.key) { - return true; - } - } - return false; - } - /** Removes and unloads plugins with a matching name from the system. diff --git a/packages/core/src/plugin.ts b/packages/core/src/plugin.ts index d1ce4358c..666eb7ddc 100644 --- a/packages/core/src/plugin.ts +++ b/packages/core/src/plugin.ts @@ -24,6 +24,7 @@ export class Plugin { this.analytics = analytics; } + // @ts-ignore update(settings: SegmentAPISettings, type: UpdateType) { // do nothing by default, user can override. }