From 3c37def422f82073e7e33d2d7c7c26c4637afdc9 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Wed, 24 Apr 2024 11:46:09 -0400 Subject: [PATCH] Refactor event factory, remove spark-md5 lib, fix hash collisions on messageId (#1045) Co-authored-by: Andrew Loyola --- .changeset/clean-trains-perform.md | 8 + .changeset/nasty-bulldogs-unite.md | 4 + packages/browser/package.json | 2 - .../__tests__/inspector.integration.test.ts | 2 +- .../src/browser/__tests__/integration.test.ts | 1 - packages/browser/src/browser/index.ts | 2 - packages/browser/src/core/analytics/index.ts | 2 +- .../src/core/events/__tests__/index.test.ts | 46 ++- packages/browser/src/core/events/index.ts | 275 +++++------------- .../node/__tests__/node-integration.test.ts | 2 +- packages/browser/src/node/index.ts | 6 +- .../validation/__tests__/index.test.ts | 78 ----- .../browser/src/plugins/validation/index.ts | 44 --- .../core/src/events/__tests__/index.test.ts | 97 ++---- packages/core/src/events/index.ts | 89 +++--- packages/core/src/user/index.ts | 6 - .../validation/__tests__/assertions.test.ts | 200 ++++++------- packages/core/src/validation/assertions.ts | 3 +- .../argument-validation.integration.test.ts | 17 ++ .../node/src/__tests__/integration.test.ts | 2 +- packages/node/src/app/event-factory.ts | 23 +- yarn.lock | 16 - 22 files changed, 326 insertions(+), 599 deletions(-) create mode 100644 .changeset/clean-trains-perform.md create mode 100644 .changeset/nasty-bulldogs-unite.md delete mode 100644 packages/browser/src/plugins/validation/__tests__/index.test.ts delete mode 100644 packages/browser/src/plugins/validation/index.ts create mode 100644 packages/node/src/__tests__/argument-validation.integration.test.ts diff --git a/.changeset/clean-trains-perform.md b/.changeset/clean-trains-perform.md new file mode 100644 index 000000000..e05846232 --- /dev/null +++ b/.changeset/clean-trains-perform.md @@ -0,0 +1,8 @@ +--- +'@segment/analytics-next': minor +--- +- Remove validation plugin +- Remove `spark-md5` dependency +- Update messageId algorithm to be consistent with node (analytics-next-[epoch time]-[uuid]) +- Browser Validation: + - Throws errors in the EventFactory (not just in a plugin) if the event is invalid diff --git a/.changeset/nasty-bulldogs-unite.md b/.changeset/nasty-bulldogs-unite.md new file mode 100644 index 000000000..559413322 --- /dev/null +++ b/.changeset/nasty-bulldogs-unite.md @@ -0,0 +1,4 @@ +--- +'@segment/analytics-core': patch +--- +Share `EventFactory` between node and browser. diff --git a/packages/browser/package.json b/packages/browser/package.json index 72e330971..63c131caf 100644 --- a/packages/browser/package.json +++ b/packages/browser/package.json @@ -57,7 +57,6 @@ "dset": "^3.1.2", "js-cookie": "3.0.1", "node-fetch": "^2.6.7", - "spark-md5": "^3.0.1", "tslib": "^2.4.1", "unfetch": "^4.1.0" }, @@ -76,7 +75,6 @@ "@types/mime": "^2.0.3", "@types/node": "^16", "@types/serve-handler": "^6.1.0", - "@types/spark-md5": "^3.0.2", "aws-sdk": "^2.814.0", "circular-dependency-plugin": "^5.2.2", "compression-webpack-plugin": "^8.0.1", diff --git a/packages/browser/src/browser/__tests__/inspector.integration.test.ts b/packages/browser/src/browser/__tests__/inspector.integration.test.ts index a5b8df713..f8f7a1d3b 100644 --- a/packages/browser/src/browser/__tests__/inspector.integration.test.ts +++ b/packages/browser/src/browser/__tests__/inspector.integration.test.ts @@ -55,7 +55,7 @@ describe('Inspector', () => { await deliveryPromise - expect(enrichedFn).toHaveBeenCalledTimes(2) // will be called once for every before or enrichment plugin. + expect(enrichedFn).toHaveBeenCalledTimes(1) // will be called once for every before or enrichment plugin. expect(deliveredFn).toHaveBeenCalledTimes(1) }) diff --git a/packages/browser/src/browser/__tests__/integration.test.ts b/packages/browser/src/browser/__tests__/integration.test.ts index bd9eeb12e..86c573003 100644 --- a/packages/browser/src/browser/__tests__/integration.test.ts +++ b/packages/browser/src/browser/__tests__/integration.test.ts @@ -650,7 +650,6 @@ describe('Dispatch', () => { "plugin_time", "plugin_time", "plugin_time", - "plugin_time", "message_delivered", "delivered", ] diff --git a/packages/browser/src/browser/index.ts b/packages/browser/src/browser/index.ts index 61b7b3f59..3480c38e1 100644 --- a/packages/browser/src/browser/index.ts +++ b/packages/browser/src/browser/index.ts @@ -22,7 +22,6 @@ import { } from '../plugins/remote-loader' import type { RoutingRule } from '../plugins/routing-middleware' import { segmentio, SegmentioSettings } from '../plugins/segmentio' -import { validation } from '../plugins/validation' import { AnalyticsBuffered, PreInitMethodCallBuffer, @@ -255,7 +254,6 @@ async function registerPlugins( ).catch(() => []) const toRegister = [ - validation, envEnrichment, ...plugins, ...legacyDestinations, diff --git a/packages/browser/src/core/analytics/index.ts b/packages/browser/src/core/analytics/index.ts index a7675c131..25d4675ef 100644 --- a/packages/browser/src/core/analytics/index.ts +++ b/packages/browser/src/core/analytics/index.ts @@ -582,7 +582,7 @@ export class Analytics normalize(msg: SegmentEvent): SegmentEvent { console.warn(deprecationWarning) - return this.eventFactory.normalize(msg) + return this.eventFactory['normalize'](msg) } get failedInitializations(): string[] { diff --git a/packages/browser/src/core/events/__tests__/index.test.ts b/packages/browser/src/core/events/__tests__/index.test.ts index d9b53250a..3104b2232 100644 --- a/packages/browser/src/core/events/__tests__/index.test.ts +++ b/packages/browser/src/core/events/__tests__/index.test.ts @@ -3,7 +3,6 @@ import { range, uniq } from 'lodash' import { EventFactory } from '..' import { getDefaultPageContext } from '../../page' import { User } from '../../user' -import { SegmentEvent, Options } from '../interfaces' describe('Event Factory', () => { let user: User @@ -59,6 +58,25 @@ describe('Event Factory', () => { const group = factory.group('coolKidsId', { coolkids: true }) expect(group.groupId).toEqual('coolKidsId') }) + + it('allows userId / anonymousId to be specified as an option', () => { + const group = factory.group('my_group_id', undefined, { + userId: 'bar', + anonymousId: 'foo', + }) + expect(group.userId).toBe('bar') + expect(group.anonymousId).toBe('foo') + }) + + it('uses userId / anonymousId from the user class (if specified)', function () { + const user = new User() + user.anonymousId = () => '123' + user.id = () => 'abc' + factory = new EventFactory(user) + const group = factory.group('my_group_id') + expect(group.userId).toBe('abc') + expect(group.anonymousId).toBe('123') + }) }) describe('page', () => { @@ -394,32 +412,6 @@ describe('Event Factory', () => { }) }) - describe('normalize', function () { - const msg: SegmentEvent = { type: 'track' } - const opts: Options = (msg.options = {}) - - describe('message', function () { - it('should merge original with normalized', function () { - msg.userId = 'user-id' - opts.integrations = { Segment: true } - const normalized = factory['normalize'](msg) - - expect(normalized.messageId?.length).toBeGreaterThanOrEqual(41) // 'ajs-next-md5(content + [UUID])' - delete normalized.messageId - - expect(normalized.timestamp).toBeInstanceOf(Date) - delete normalized.timestamp - - expect(normalized).toStrictEqual({ - integrations: { Segment: true }, - type: 'track', - userId: 'user-id', - context: defaultContext, - }) - }) - }) - }) - describe('Page context augmentation', () => { // minimal tests -- more tests should be specifically around addPageContext factory = new EventFactory(new User()) diff --git a/packages/browser/src/core/events/index.ts b/packages/browser/src/core/events/index.ts index b213cc55b..d85257d5f 100644 --- a/packages/browser/src/core/events/index.ts +++ b/packages/browser/src/core/events/index.ts @@ -1,5 +1,4 @@ import { v4 as uuid } from '@lukeed/uuid' -import { dset } from 'dset' import { ID, User } from '../user' import { Options, @@ -8,35 +7,61 @@ import { Traits, SegmentEvent, } from './interfaces' -import md5 from 'spark-md5' import { addPageContext, PageContext } from '../page' +import { CoreEventFactory } from '@segment/analytics-core' export * from './interfaces' -export class EventFactory { - constructor(public user: User) {} +export class EventFactory extends CoreEventFactory { + user: User + constructor(user: User) { + super({ + createMessageId: () => `ajs-next-${Date.now()}-${uuid()}`, + onEventMethodCall: ({ options }) => { + this.maybeUpdateAnonId(options) + }, + onFinishedEvent: (event) => { + this.addIdentity(event) + return event + }, + }) + this.user = user + } + + /** + * Updates the anonymousId *globally* if it's provided in the options. + * This should generally be done in the identify method, but some customers rely on this. + */ + private maybeUpdateAnonId(options: Options | undefined): void { + options?.anonymousId && this.user.anonymousId(options.anonymousId) + } + + /** + * add user id / anonymous id to the event + */ + private addIdentity(event: SegmentEvent) { + if (this.user.id()) { + event.userId = this.user.id() + } - track( + if (this.user.anonymousId()) { + event.anonymousId = this.user.anonymousId() + } + } + + override track( event: string, properties?: EventProperties, options?: Options, globalIntegrations?: Integrations, pageCtx?: PageContext ): SegmentEvent { - return this.normalize( - { - ...this.baseEvent(), - event, - type: 'track' as const, - properties, - options: { ...options }, - integrations: { ...globalIntegrations }, - }, - pageCtx - ) + const ev = super.track(event, properties, options, globalIntegrations) + addPageContext(ev, pageCtx) + return ev } - page( + override page( category: string | null, page: string | null, properties?: EventProperties, @@ -44,33 +69,18 @@ export class EventFactory { globalIntegrations?: Integrations, pageCtx?: PageContext ): SegmentEvent { - const event: Partial = { - type: 'page' as const, - properties: { ...properties }, - options: { ...options }, - integrations: { ...globalIntegrations }, - } - - if (category !== null) { - event.category = category - event.properties = event.properties ?? {} - event.properties.category = category - } - - if (page !== null) { - event.name = page - } - - return this.normalize( - { - ...this.baseEvent(), - ...event, - } as SegmentEvent, - pageCtx + const ev = super.page( + category, + page, + properties, + options, + globalIntegrations ) + addPageContext(ev, pageCtx) + return ev } - screen( + override screen( category: string | null, screen: string | null, properties?: EventProperties, @@ -78,193 +88,50 @@ export class EventFactory { globalIntegrations?: Integrations, pageCtx?: PageContext ): SegmentEvent { - const event: Partial = { - type: 'screen' as const, - properties: { ...properties }, - options: { ...options }, - integrations: { ...globalIntegrations }, - } - - if (category !== null) { - event.category = category - } - - if (screen !== null) { - event.name = screen - } - return this.normalize( - { - ...this.baseEvent(), - ...event, - } as SegmentEvent, - pageCtx + const ev = super.screen( + category, + screen, + properties, + options, + globalIntegrations ) + addPageContext(ev, pageCtx) + return ev } - identify( + override identify( userId: ID, traits?: Traits, options?: Options, globalIntegrations?: Integrations, pageCtx?: PageContext ): SegmentEvent { - return this.normalize( - { - ...this.baseEvent(), - type: 'identify' as const, - userId, - traits, - options: { ...options }, - integrations: { ...globalIntegrations }, - }, - pageCtx - ) + const ev = super.identify(userId, traits, options, globalIntegrations) + addPageContext(ev, pageCtx) + return ev } - group( + override group( groupId: ID, traits?: Traits, options?: Options, globalIntegrations?: Integrations, pageCtx?: PageContext ): SegmentEvent { - return this.normalize( - { - ...this.baseEvent(), - type: 'group' as const, - traits, - options: { ...options }, - integrations: { ...globalIntegrations }, - groupId, - }, - pageCtx - ) + const ev = super.group(groupId, traits, options, globalIntegrations) + addPageContext(ev, pageCtx) + return ev } - alias( + override alias( to: string, from: string | null, options?: Options, globalIntegrations?: Integrations, pageCtx?: PageContext ): SegmentEvent { - const base: Partial = { - userId: to, - type: 'alias' as const, - options: { ...options }, - integrations: { ...globalIntegrations }, - } - - if (from !== null) { - base.previousId = from - } - - if (to === undefined) { - return this.normalize({ - ...base, - ...this.baseEvent(), - } as SegmentEvent) - } - - return this.normalize( - { - ...this.baseEvent(), - ...base, - } as SegmentEvent, - pageCtx - ) - } - - private baseEvent(): Partial { - const base: Partial = { - integrations: {}, - options: {}, - } - - const user = this.user - - if (user.id()) { - base.userId = user.id() - } - - if (user.anonymousId()) { - base.anonymousId = user.anonymousId() - } - - return base - } - - /** - * Builds the context part of an event based on "foreign" keys that - * are provided in the `Options` parameter for an Event - */ - private context(event: SegmentEvent): [object, object] { - const optionsKeys = ['integrations', 'anonymousId', 'timestamp', 'userId'] - - const options = event.options ?? {} - delete options['integrations'] - - const providedOptionsKeys = Object.keys(options) - - const context = event.options?.context ?? {} - const overrides = {} - - providedOptionsKeys.forEach((key) => { - if (key === 'context') { - return - } - - if (optionsKeys.includes(key)) { - dset(overrides, key, options[key]) - } else { - dset(context, key, options[key]) - } - }) - - return [context, overrides] - } - - public normalize(event: SegmentEvent, pageCtx?: PageContext): SegmentEvent { - // set anonymousId globally if we encounter an override - //segment.com/docs/connections/sources/catalog/libraries/website/javascript/identity/#override-the-anonymous-id-using-the-options-object - event.options?.anonymousId && - this.user.anonymousId(event.options.anonymousId) - - const integrationBooleans = Object.keys(event.integrations ?? {}).reduce( - (integrationNames, name) => { - return { - ...integrationNames, - [name]: Boolean(event.integrations?.[name]), - } - }, - {} as Record - ) - - // This is pretty trippy, but here's what's going on: - // - a) We don't pass initial integration options as part of the event, only if they're true or false - // - b) We do accept per integration overrides (like integrations.Amplitude.sessionId) at the event level - // Hence the need to convert base integration options to booleans, but maintain per event integration overrides - const allIntegrations = { - // Base config integrations object as booleans - ...integrationBooleans, - - // Per event overrides, for things like amplitude sessionId, for example - ...event.options?.integrations, - } - - const [context, overrides] = this.context(event) - const { options, ...rest } = event - - const newEvent: SegmentEvent = { - timestamp: new Date(), - ...rest, - context, - integrations: allIntegrations, - ...overrides, - messageId: 'ajs-next-' + md5.hash(JSON.stringify(event) + uuid()), - } - addPageContext(newEvent, pageCtx) - - return newEvent + const ev = super.alias(to, from, options, globalIntegrations) + addPageContext(ev, pageCtx) + return ev } } diff --git a/packages/browser/src/node/__tests__/node-integration.test.ts b/packages/browser/src/node/__tests__/node-integration.test.ts index d67a59183..6ba2bc8ae 100644 --- a/packages/browser/src/node/__tests__/node-integration.test.ts +++ b/packages/browser/src/node/__tests__/node-integration.test.ts @@ -8,7 +8,7 @@ describe('Initialization', () => { writeKey, }) - expect(analytics.queue.plugins.length).toBe(2) + expect(analytics.queue.plugins.length).toBe(1) const ajsNodeXt = analytics.queue.plugins.find( (xt) => xt.name === 'analytics-node-next' diff --git a/packages/browser/src/node/index.ts b/packages/browser/src/node/index.ts index 5362979a5..937491efd 100644 --- a/packages/browser/src/node/index.ts +++ b/packages/browser/src/node/index.ts @@ -1,6 +1,5 @@ import { Analytics } from '../core/analytics' import { Context } from '../core/context' -import { validation } from '../plugins/validation' import { analyticsNode } from '../plugins/analytics-node' import { Plugin } from '../core/plugin' import { EventQueue } from '../core/queue/event-queue' @@ -28,10 +27,7 @@ export class AnalyticsNode { version: 'latest', } - const ctx = await analytics.register( - validation, - analyticsNode(nodeSettings) - ) + const ctx = await analytics.register(analyticsNode(nodeSettings)) analytics.emit('initialize', settings, cookieOptions ?? {}) return [analytics, ctx] diff --git a/packages/browser/src/plugins/validation/__tests__/index.test.ts b/packages/browser/src/plugins/validation/__tests__/index.test.ts deleted file mode 100644 index 127f8dc29..000000000 --- a/packages/browser/src/plugins/validation/__tests__/index.test.ts +++ /dev/null @@ -1,78 +0,0 @@ -import { validation } from '..' -import { Context } from '../../../core/context' -import { SegmentEvent } from '../../../core/events' - -const validEvent: SegmentEvent = { - type: 'track', - anonymousId: 'abc', - event: 'test', - properties: {}, - traits: {}, -} - -describe('validation', () => { - ;['track', 'identify', 'group', 'page', 'alias'].forEach((method) => { - // @ts-ignore - const validationFn: Function = validation[method] as any - describe(method, () => { - it('validates that the `event` exists', async () => { - try { - // @ts-ignore - await validationFn(new Context()) - } catch (err) { - expect(err).toBeTruthy() - } - expect.assertions(1) - }) - - it('validates that `event.event` exists', async () => { - if (method === 'track') { - try { - await validationFn( - new Context({ - ...validEvent, - event: undefined, - }) - ) - } catch (err) { - expect(err).toBeTruthy() - } - expect.assertions(1) - } - }) - - it('validates that `properties` or `traits` are objects', async () => { - if (method === 'alias') { - return - } - try { - await validationFn( - new Context({ - ...validEvent, - properties: undefined, - traits: undefined, - }) - ) - } catch (err) { - expect(err).toBeTruthy() - } - expect.assertions(1) - }) - - it('validates that it contains an user', async () => { - try { - await validationFn( - new Context({ - ...validEvent, - userId: undefined, - anonymousId: undefined, - }) - ) - } catch (err) { - expect(err).toBeTruthy() - } - expect.assertions(1) - }) - }) - }) -}) diff --git a/packages/browser/src/plugins/validation/index.ts b/packages/browser/src/plugins/validation/index.ts deleted file mode 100644 index c9080f0d1..000000000 --- a/packages/browser/src/plugins/validation/index.ts +++ /dev/null @@ -1,44 +0,0 @@ -import type { Plugin } from '../../core/plugin' -import type { Context } from '../../core/context' -import { - assertUserIdentity, - isPlainObject, - ValidationError, - assertEventExists, - assertEventType, - assertTrackEventName, -} from '@segment/analytics-core' - -function validate(ctx: Context): Context { - const event = ctx.event - assertEventExists(event) - assertEventType(event) - - if (event.type === 'track') { - assertTrackEventName(event) - } - - const props = event.properties ?? event.traits - if (event.type !== 'alias' && !isPlainObject(props)) { - throw new ValidationError('.properties', 'is not an object') - } - - assertUserIdentity(event) - return ctx -} - -export const validation: Plugin = { - name: 'Event Validation', - type: 'before', - version: '1.0.0', - - isLoaded: () => true, - load: () => Promise.resolve(), - - track: validate, - identify: validate, - page: validate, - alias: validate, - group: validate, - screen: validate, -} diff --git a/packages/core/src/events/__tests__/index.test.ts b/packages/core/src/events/__tests__/index.test.ts index 27c482a5c..52162033b 100644 --- a/packages/core/src/events/__tests__/index.test.ts +++ b/packages/core/src/events/__tests__/index.test.ts @@ -1,23 +1,22 @@ -import { EventFactory } from '..' -import { User } from '../../user' +import { CoreEventFactory } from '..' import { CoreSegmentEvent } from '../..' import { isDate } from 'lodash' describe('Event Factory', () => { - let factory: EventFactory - let user: User + let factory: CoreEventFactory const shoes = { product: 'shoes', total: '$35', category: 'category' } const shopper = { totalSpent: 100 } beforeEach(() => { - user = { - anonymousId: () => undefined, - id: () => 'foo', + class TestEventFactory extends CoreEventFactory { + constructor() { + super({ + createMessageId: () => 'foo', + }) + } } - factory = new EventFactory({ - user, - createMessageId: () => 'foo', - }) + + factory = new TestEventFactory() }) describe('alias', () => { @@ -46,7 +45,6 @@ describe('Event Factory', () => { expect(group.traits).toEqual({ coolkids: true }) expect(group.type).toEqual('group') expect(group.event).toBeUndefined() - expect(group.userId).toBe('foo') expect(group.anonymousId).toBeUndefined() }) @@ -60,37 +58,6 @@ describe('Event Factory', () => { expect(group.groupId).toEqual('coolKidsId') }) - it('allows userId / anonymousId to be specified as an option', () => { - const group = factory.group('my_group_id', undefined, { - userId: 'bar', - anonymousId: 'foo', - }) - expect(group.userId).toBe('bar') - expect(group.anonymousId).toBe('foo') - }) - - it('allows userId / anonymousId to be overridden', function () { - const group = factory.group('my_group_id', undefined, { - userId: 'bar', - anonymousId: 'foo', - }) - expect(group.userId).toBe('bar') - expect(group.anonymousId).toBe('foo') - }) - - it('uses userId / anonymousId from the user class (if specified)', function () { - factory = new EventFactory({ - createMessageId: () => 'foo', - user: { - id: () => 'abc', - anonymousId: () => '123', - }, - }) - const group = factory.group('my_group_id') - expect(group.userId).toBe('abc') - expect(group.anonymousId).toBe('123') - }) - test('should be capable of working with empty traits', () => { expect(() => factory.group('foo')).not.toThrow() expect(() => factory.group('foo', null as any)).not.toThrow() @@ -99,7 +66,9 @@ describe('Event Factory', () => { describe('page', () => { test('creates page events', () => { - const page = factory.page('category', 'name') + const page = factory.page('category', 'name', undefined, { + userId: 'foo', + }) expect(page.traits).toBeUndefined() expect(page.type).toEqual('page') expect(page.event).toBeUndefined() @@ -108,12 +77,12 @@ describe('Event Factory', () => { }) it('accepts properties', () => { - const page = factory.page('category', 'name', shoes) + const page = factory.page('category', 'name', shoes, { userId: 'foo' }) expect(page.properties).toEqual(shoes) }) it('ignores category and page if not passed in', () => { - const page = factory.page(null, null) + const page = factory.page(null, null, undefined, { userId: 'foo' }) expect(page.category).toBeUndefined() expect(page.name).toBeUndefined() }) @@ -145,7 +114,7 @@ describe('Event Factory', () => { }) test('creates track events', () => { - const track = factory.track('Order Completed', shoes) + const track = factory.track('Order Completed', shoes, { userId: 'foo' }) expect(track.event).toEqual('Order Completed') expect(track.properties).toEqual(shoes) expect(track.traits).toBeUndefined() @@ -153,31 +122,18 @@ describe('Event Factory', () => { }) test('adds a message id', () => { - const track = factory.track('Order Completed', shoes) + const track = factory.track('Order Completed', shoes, { userId: 'foo' }) expect(typeof track.messageId).toBe('string') }) test('adds a timestamp', () => { - const track = factory.track('Order Completed', shoes) + const track = factory.track('Order Completed', shoes, { userId: 'foo' }) expect(track.timestamp).toBeInstanceOf(Date) }) - test('sets an user id', () => { - user.id = () => '007' - - const track = factory.track('Order Completed', shoes) - expect(track.userId).toEqual('007') - }) - - test('sets an anonymous id', () => { - user.anonymousId = () => 'foo' - - const track = factory.track('Order Completed', shoes) - expect(track.anonymousId).toEqual(user.anonymousId()) - }) - test('sets options in the context', () => { const track = factory.track('Order Completed', shoes, { + userId: 'foo', opt1: true, }) expect(track.context).toEqual({ opt1: true }) @@ -187,7 +143,9 @@ describe('Event Factory', () => { const track = factory.track( 'Order Completed', shoes, - {}, + { + userId: 'foo', + }, { amplitude: false, } @@ -201,6 +159,7 @@ describe('Event Factory', () => { 'Order Completed', shoes, { + userId: 'foo', opt1: true, integrations: { amplitude: false, @@ -223,6 +182,7 @@ describe('Event Factory', () => { 'Order Completed', shoes, { + userId: 'foo', integrations: { Amplitude: { sessionId: 'session_123', @@ -255,6 +215,7 @@ describe('Event Factory', () => { test('should move foreign options into `context`', () => { const track = factory.track('Order Completed', shoes, { + userId: 'foo', opt1: true, opt2: '🥝', integrations: { @@ -267,6 +228,7 @@ describe('Event Factory', () => { test('should not move known options into `context`', () => { const track = factory.track('Order Completed', shoes, { + userId: 'foo', opt1: true, opt2: '🥝', integrations: { @@ -291,6 +253,7 @@ describe('Event Factory', () => { test('accepts a timestamp', () => { const timestamp = new Date() const track = factory.track('Order Completed', shoes, { + userId: 'foo', timestamp, }) @@ -300,6 +263,7 @@ describe('Event Factory', () => { test('accepts traits', () => { const track = factory.track('Order Completed', shoes, { + userId: 'foo', traits: { cell: '📱', }, @@ -313,6 +277,7 @@ describe('Event Factory', () => { test('accepts a context object', () => { const track = factory.track('Order Completed', shoes, { + userId: 'foo', context: { library: { name: 'ajs-next', @@ -331,6 +296,7 @@ describe('Event Factory', () => { test('merges a context object', () => { const track = factory.track('Order Completed', shoes, { + userId: 'foo', foreignProp: '🇧🇷', context: { innerProp: '👻', @@ -354,6 +320,7 @@ describe('Event Factory', () => { test('accepts a messageId', () => { const messageId = 'business-id-123' const track = factory.track('Order Completed', shoes, { + userId: 'foo', messageId, }) @@ -365,7 +332,7 @@ describe('Event Factory', () => { const event = factory.track( 'Order Completed', { ...shoes }, - { timestamp: undefined, traits: { foo: 123 } } + { userId: 'foo', timestamp: undefined, traits: { foo: 123 } } ) expect(typeof event.timestamp).toBe('object') diff --git a/packages/core/src/events/index.ts b/packages/core/src/events/index.ts index 2c7d80ee0..8c772f536 100644 --- a/packages/core/src/events/index.ts +++ b/packages/core/src/events/index.ts @@ -1,6 +1,6 @@ export * from './interfaces' import { dset } from 'dset' -import { ID, User } from '../user' +import { ID } from '../user' import { Integrations, EventProperties, @@ -11,25 +11,56 @@ import { GroupTraits, } from './interfaces' import { pickBy } from '../utils/pick' -import { validateEvent } from '../validation/assertions' import type { RemoveIndexSignature } from '../utils/ts-helpers' +import { validateEvent } from '../validation/assertions' + +export type EventMethodCallHook = ({ + type, + options, +}: { + type: 'track' | 'identify' | 'page' | 'group' | 'alias' | 'screen' + options?: CoreOptions +}) => void + +export type EventHook = (event: CoreSegmentEvent) => void -interface EventFactorySettings { +export interface EventFactorySettings { + /** + * Universal `messageId` builder for all events (these must be unique) + */ createMessageId: () => string - user?: User + /** + * Hook to do something with an event right before they are returned from the factory. + * This includes event modification or additional validation. + */ + onFinishedEvent?: EventHook + /** + * Hook whenever an event method is called (track, page, etc.) + * Can be used to update Options (or just listen) + */ + onEventMethodCall?: EventMethodCallHook } /** - * This is currently only used by node.js, but the original idea was to have something that could be shared between browser and node. - * Unfortunately, there are some differences in the way the two environments handle events, so this is not currently shared. + * Internal settings object that is used internally by the factory */ -export class EventFactory { - createMessageId: EventFactorySettings['createMessageId'] - user?: User +class InternalEventFactorySettings { + public createMessageId: EventFactorySettings['createMessageId'] + public onEventMethodCall: EventMethodCallHook + public onFinishedEvent: EventHook - constructor(settings: EventFactorySettings) { - this.user = settings.user + constructor(public settings: EventFactorySettings) { this.createMessageId = settings.createMessageId + this.onEventMethodCall = settings.onEventMethodCall ?? (() => {}) + this.onFinishedEvent = settings.onFinishedEvent ?? (() => {}) + } +} + +export abstract class CoreEventFactory { + private settings: InternalEventFactorySettings + + constructor(settings: EventFactorySettings) { + this.settings = new InternalEventFactorySettings(settings) } track( @@ -38,6 +69,7 @@ export class EventFactory { options?: CoreOptions, globalIntegrations?: Integrations ) { + this.settings.onEventMethodCall({ type: 'track', options }) return this.normalize({ ...this.baseEvent(), event, @@ -55,6 +87,7 @@ export class EventFactory { options?: CoreOptions, globalIntegrations?: Integrations ): CoreSegmentEvent { + this.settings.onEventMethodCall({ type: 'page', options }) const event: CoreSegmentEvent = { type: 'page', properties: { ...properties }, @@ -85,6 +118,7 @@ export class EventFactory { options?: CoreOptions, globalIntegrations?: Integrations ): CoreSegmentEvent { + this.settings.onEventMethodCall({ type: 'screen', options }) const event: CoreSegmentEvent = { type: 'screen', properties: { ...properties }, @@ -112,6 +146,7 @@ export class EventFactory { options?: CoreOptions, globalIntegrations?: Integrations ): CoreSegmentEvent { + this.settings.onEventMethodCall({ type: 'identify', options }) return this.normalize({ ...this.baseEvent(), type: 'identify', @@ -128,6 +163,7 @@ export class EventFactory { options?: CoreOptions, globalIntegrations?: Integrations ): CoreSegmentEvent { + this.settings.onEventMethodCall({ type: 'group', options }) return this.normalize({ ...this.baseEvent(), type: 'group', @@ -144,6 +180,7 @@ export class EventFactory { options?: CoreOptions, globalIntegrations?: Integrations ): CoreSegmentEvent { + this.settings.onEventMethodCall({ type: 'alias', options }) const base: CoreSegmentEvent = { userId: to, type: 'alias', @@ -169,24 +206,10 @@ export class EventFactory { } private baseEvent(): Partial { - const base: Partial = { + return { integrations: {}, options: {}, } - - if (!this.user) return base - - const user = this.user - - if (user.id()) { - base.userId = user.id() - } - - if (user.anonymousId()) { - base.anonymousId = user.anonymousId() - } - - return base } /** @@ -232,7 +255,7 @@ export class EventFactory { return [context, eventOverrides] } - public normalize(event: CoreSegmentEvent): CoreSegmentEvent { + private normalize(event: CoreSegmentEvent): CoreSegmentEvent { const integrationBooleans = Object.keys(event.integrations ?? {}).reduce( (integrationNames, name) => { return { @@ -266,20 +289,18 @@ export class EventFactory { const { options, ...rest } = event - const body = { + const evt: CoreSegmentEvent = { timestamp: new Date(), ...rest, - integrations: allIntegrations, context, + integrations: allIntegrations, ...overrides, + messageId: options.messageId || this.settings.createMessageId(), } - const evt: CoreSegmentEvent = { - ...body, - messageId: options.messageId || this.createMessageId(), - } - + this.settings.onFinishedEvent(evt) validateEvent(evt) + return evt } } diff --git a/packages/core/src/user/index.ts b/packages/core/src/user/index.ts index 31c472d2d..b16268ce6 100644 --- a/packages/core/src/user/index.ts +++ b/packages/core/src/user/index.ts @@ -1,7 +1 @@ export type ID = string | null | undefined - -// TODO: this is a base user -export interface User { - id(): ID - anonymousId(): ID -} diff --git a/packages/core/src/validation/__tests__/assertions.test.ts b/packages/core/src/validation/__tests__/assertions.test.ts index df4735c14..37aa0a559 100644 --- a/packages/core/src/validation/__tests__/assertions.test.ts +++ b/packages/core/src/validation/__tests__/assertions.test.ts @@ -1,5 +1,5 @@ import { CoreSegmentEvent } from '../../events' -import { validateEvent } from '../assertions' +import { assertUserIdentity, validateEvent } from '../assertions' const baseEvent: Partial = { userId: 'foo', @@ -7,6 +7,83 @@ const baseEvent: Partial = { messageId: 'foo', } +describe(assertUserIdentity, () => { + test('should pass if either a userID/anonymousID/previousID/groupID are defined', () => { + expect(() => + assertUserIdentity({ + ...baseEvent, + type: 'track', + properties: {}, + userId: undefined, + anonymousId: 'foo', + }) + ).not.toThrow() + + expect(() => + assertUserIdentity({ + ...baseEvent, + type: 'identify', + traits: {}, + userId: 'foo', + anonymousId: undefined, + }) + ).not.toThrow() + + expect(() => + assertUserIdentity({ + ...baseEvent, + type: 'alias', + previousId: 'foo', + userId: undefined, + anonymousId: undefined, + }) + ).not.toThrow() + + expect(() => + assertUserIdentity({ + ...baseEvent, + type: 'group', + traits: {}, + groupId: 'foo', + }) + ).not.toThrow() + }) + + test('should fail if ID is _not_ string', () => { + expect(() => + assertUserIdentity({ + ...baseEvent, + type: 'track', + properties: {}, + userId: undefined, + anonymousId: 123 as any, + }) + ).toThrowError(/string/) + + expect(() => + assertUserIdentity({ + ...baseEvent, + type: 'track', + properties: {}, + userId: undefined, + anonymousId: 123 as any, + }) + ).toThrowError(/string/) + }) + + test('should handle null as well as undefined', () => { + expect(() => + assertUserIdentity({ + ...baseEvent, + type: 'track', + properties: {}, + userId: undefined, + anonymousId: null, + }) + ).toThrowError(/nil/i) + }) +}) + describe(validateEvent, () => { test('should be capable of working with empty properties and traits', () => { expect(() => validateEvent(undefined)).toThrowError() @@ -79,104 +156,27 @@ describe(validateEvent, () => { ).not.toThrow() }) - describe('User Validation', () => { - test('should pass if either a userID/anonymousID/previousID/groupID are defined', () => { - expect(() => - validateEvent({ - ...baseEvent, - type: 'track', - properties: {}, - userId: undefined, - anonymousId: 'foo', - }) - ).not.toThrow() - - expect(() => - validateEvent({ - ...baseEvent, - type: 'identify', - traits: {}, - userId: 'foo', - anonymousId: undefined, - }) - ).not.toThrow() - - expect(() => - validateEvent({ - ...baseEvent, - type: 'alias', - previousId: 'foo', - userId: undefined, - anonymousId: undefined, - }) - ).not.toThrow() - - expect(() => - validateEvent({ - ...baseEvent, - type: 'group', - traits: {}, - groupId: 'foo', - }) - ).not.toThrow() - }) - - test('should fail if ID is _not_ string', () => { - expect(() => - validateEvent({ - ...baseEvent, - type: 'track', - properties: {}, - userId: undefined, - anonymousId: 123 as any, - }) - ).toThrowError(/string/) - - expect(() => - validateEvent({ - ...baseEvent, - type: 'track', - properties: {}, - userId: undefined, - anonymousId: 123 as any, - }) - ).toThrowError(/string/) - }) - - test('should handle null as well as undefined', () => { - expect(() => - validateEvent({ - ...baseEvent, - type: 'track', - properties: {}, - userId: undefined, - anonymousId: null, - }) - ).toThrowError(/nil/i) - }) - - test('should fail if messageId is _not_ string', () => { - expect(() => - validateEvent({ - ...baseEvent, - type: 'track', - properties: {}, - userId: undefined, - anonymousId: 'foo', - messageId: 'bar', - }) - ).not.toThrow() - - expect(() => - validateEvent({ - ...baseEvent, - type: 'track', - properties: {}, - userId: undefined, - anonymousId: 'foo', - messageId: 123 as any, - }) - ).toThrow(/messageId/) - }) + test('should fail if messageId is _not_ string', () => { + expect(() => + validateEvent({ + ...baseEvent, + type: 'track', + properties: {}, + userId: undefined, + anonymousId: 'foo', + messageId: 'bar', + }) + ).not.toThrow() + + expect(() => + validateEvent({ + ...baseEvent, + type: 'track', + properties: {}, + userId: undefined, + anonymousId: 'foo', + messageId: 123 as any, + }) + ).toThrow(/messageId/) }) }) diff --git a/packages/core/src/validation/assertions.ts b/packages/core/src/validation/assertions.ts index e563a6a0e..b40e68e01 100644 --- a/packages/core/src/validation/assertions.ts +++ b/packages/core/src/validation/assertions.ts @@ -6,6 +6,7 @@ const stringError = 'is not a string' const objError = 'is not an object' const nilError = 'is nil' +// user identity check could hypothetically could be used in the browser event factory, but not 100% sure -- so this is node only for now export function assertUserIdentity(event: CoreSegmentEvent): void { const USER_FIELD_NAME = '.userId/anonymousId/previousId/groupId' @@ -74,6 +75,4 @@ export function validateEvent(event?: CoreSegmentEvent | null) { if (['group', 'identify'].includes(event.type)) { assertTraits(event) } - - assertUserIdentity(event) } diff --git a/packages/node/src/__tests__/argument-validation.integration.test.ts b/packages/node/src/__tests__/argument-validation.integration.test.ts new file mode 100644 index 000000000..581b03084 --- /dev/null +++ b/packages/node/src/__tests__/argument-validation.integration.test.ts @@ -0,0 +1,17 @@ +import { createTestAnalytics } from './test-helpers/create-test-analytics' + +// This is a smoke test. +// More detailed tests can be found in packages/core/src/validation/__tests__/assertions.test.ts +describe('Argument validation', () => { + it('should throw an error if userId/anonId/groupId is not specified', async () => { + const analytics = createTestAnalytics() + + expect(() => + analytics.track({ + event: 'foo', + anonymousId: undefined as any, + userId: undefined as any, + }) + ).toThrow(/userId/) + }) +}) diff --git a/packages/node/src/__tests__/integration.test.ts b/packages/node/src/__tests__/integration.test.ts index 2554c115d..a555e89be 100644 --- a/packages/node/src/__tests__/integration.test.ts +++ b/packages/node/src/__tests__/integration.test.ts @@ -51,7 +51,7 @@ describe('Settings / Configuration Init', () => { describe('Error handling', () => { it('surfaces property thrown errors', async () => { const analytics = createTestAnalytics() - expect(() => analytics.track({} as any)).toThrowError(/event/i) + expect(() => analytics.track({} as any)).toThrowError() }) it('should emit on an error', async () => { diff --git a/packages/node/src/app/event-factory.ts b/packages/node/src/app/event-factory.ts index 606adb64a..a25e01219 100644 --- a/packages/node/src/app/event-factory.ts +++ b/packages/node/src/app/event-factory.ts @@ -1,20 +1,25 @@ -import { EventFactory } from '@segment/analytics-core' +import { assertUserIdentity, CoreEventFactory } from '@segment/analytics-core' import { createMessageId } from '../lib/get-message-id' import { SegmentEvent } from './types' // use declaration merging to downcast CoreSegmentEvent without adding any runtime code. // if/when we decide to add an actual implementation to NodeEventFactory that actually changes the event shape, we can remove this. export interface NodeEventFactory { - alias(...args: Parameters): SegmentEvent - group(...args: Parameters): SegmentEvent - identify(...args: Parameters): SegmentEvent - track(...args: Parameters): SegmentEvent - page(...args: Parameters): SegmentEvent - screen(...args: Parameters): SegmentEvent + alias(...args: Parameters): SegmentEvent + group(...args: Parameters): SegmentEvent + identify(...args: Parameters): SegmentEvent + track(...args: Parameters): SegmentEvent + page(...args: Parameters): SegmentEvent + screen(...args: Parameters): SegmentEvent } -export class NodeEventFactory extends EventFactory { +export class NodeEventFactory extends CoreEventFactory { constructor() { - super({ createMessageId }) + super({ + createMessageId, + onFinishedEvent: (event) => { + assertUserIdentity(event) + }, + }) } } diff --git a/yarn.lock b/yarn.lock index f63596791..7eb180137 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3977,7 +3977,6 @@ __metadata: "@types/mime": ^2.0.3 "@types/node": ^16 "@types/serve-handler": ^6.1.0 - "@types/spark-md5": ^3.0.2 aws-sdk: ^2.814.0 circular-dependency-plugin: ^5.2.2 compression-webpack-plugin: ^8.0.1 @@ -3999,7 +3998,6 @@ __metadata: playwright: ^1.28.1 serve-handler: ^6.1.3 size-limit: ^7.0.8 - spark-md5: ^3.0.1 terser-webpack-plugin: ^5.1.4 ts-loader: ^9.1.1 ts-node: ^10.8.0 @@ -6099,13 +6097,6 @@ __metadata: languageName: node linkType: hard -"@types/spark-md5@npm:^3.0.2": - version: 3.0.2 - resolution: "@types/spark-md5@npm:3.0.2" - checksum: daa98b1adbda0c0ba6996dbaf2472cb09d72a828d01c114f67dbab1e7f0ffc23268680c64978a88db46ee1948282d01b0ea0ecd5cfdbac0c6c4eb9c9de76a12a - languageName: node - linkType: hard - "@types/stack-utils@npm:^2.0.0": version: 2.0.0 resolution: "@types/stack-utils@npm:2.0.0" @@ -19908,13 +19899,6 @@ __metadata: languageName: node linkType: hard -"spark-md5@npm:^3.0.1": - version: 3.0.1 - resolution: "spark-md5@npm:3.0.1" - checksum: 613a7f4e20929b832406ef2ed925d696d4d004455360b07ba1a829975fc12a005d745db370cb261ecc827c3dedf3b431a9337f4bab1f1954df776f41a05aaabe - languageName: node - linkType: hard - "spawn-command@npm:^0.0.2-1": version: 0.0.2-1 resolution: "spawn-command@npm:0.0.2-1"