Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support destination filters for device mode action destinations #597

Merged
merged 21 commits into from
Oct 12, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silver-mugs-provide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-next': major
danieljackins marked this conversation as resolved.
Show resolved Hide resolved
---

Added destination filter support to action destinations
2 changes: 1 addition & 1 deletion packages/browser/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
"size-limit": [
{
"path": "dist/umd/index.js",
"limit": "25.95 KB"
"limit": "27 KB"
}
],
"dependencies": {
Expand Down
27 changes: 25 additions & 2 deletions packages/browser/src/browser/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,13 @@ function hasLegacyDestinations(settings: LegacySettings): boolean {
)
}

function hasTsubMiddleware(settings: LegacySettings): boolean {
return (
getProcessEnv().NODE_ENV !== 'test' &&
(settings.middlewareSettings?.routingRules?.length ?? 0) > 0
danieljackins marked this conversation as resolved.
Show resolved Hide resolved
)
}

/**
* With AJS classic, we allow users to call setAnonymousId before the library initialization.
* This is important because some of the destinations will use the anonymousId during the initialization,
Expand Down Expand Up @@ -146,11 +153,26 @@ async function registerPlugins(
options: InitOptions,
plugins: Plugin[]
): Promise<Context> {
const tsubMiddleware = hasTsubMiddleware(legacySettings)
? await import(
/* webpackChunkName: "tsub-middleware" */ '../plugins/routing-middleware'
).then((mod) => {
return mod.tsubMiddleware(
legacySettings.middlewareSettings!.routingRules
)
})
: undefined

const legacyDestinations = hasLegacyDestinations(legacySettings)
? await import(
/* webpackChunkName: "ajs-destination" */ '../plugins/ajs-destination'
).then((mod) => {
return mod.ajsDestinations(legacySettings, analytics.integrations, opts)
return mod.ajsDestinations(
legacySettings,
analytics.integrations,
opts,
tsubMiddleware
)
})
: []

Expand All @@ -175,7 +197,8 @@ async function registerPlugins(
legacySettings,
analytics.integrations,
mergedSettings,
options.obfuscate
options.obfuscate,
tsubMiddleware
).catch(() => [])

const toRegister = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,10 @@ describe('loading ajsDestinations', () => {
})

it('adds a tsub middleware for matching rules', () => {
const destinations = ajsDestinations(cdnResponse)
const middleware = tsubMiddleware(
cdnResponse.middlewareSettings!.routingRules
)
const destinations = ajsDestinations(cdnResponse, {}, {}, middleware)
const amplitude = destinations.find((d) => d.name === 'Amplitude')
expect(amplitude?.middleware.length).toBe(1)
})
Expand Down
8 changes: 4 additions & 4 deletions packages/browser/src/plugins/ajs-destination/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
applyDestinationMiddleware,
DestinationMiddlewareFunction,
} from '../middleware'
import { tsubMiddleware } from '../routing-middleware'
import { loadIntegration, resolveVersion, unloadIntegration } from './loader'
import { LegacyIntegration } from './types'

Expand Down Expand Up @@ -303,7 +302,8 @@ export class LegacyDestination implements Plugin {
export function ajsDestinations(
settings: LegacySettings,
globalIntegrations: Integrations = {},
options: InitOptions = {}
options: InitOptions = {},
routingMiddleware?: DestinationMiddlewareFunction
): LegacyDestination[] {
if (isServer()) {
return []
Expand All @@ -315,7 +315,7 @@ export function ajsDestinations(
}

const routingRules = settings.middlewareSettings?.routingRules ?? []
const routingMiddleware = tsubMiddleware(routingRules)
// const routingMiddleware = tsubMiddleware(routingRules)
danieljackins marked this conversation as resolved.
Show resolved Hide resolved

// merged remote CDN settings with user provided options
const integrationOptions = mergedOptions(settings, options ?? {}) as Record<
Expand Down Expand Up @@ -362,7 +362,7 @@ export function ajsDestinations(
const routing = routingRules.filter(
(rule) => rule.destinationName === name
)
if (routing.length > 0) {
if (routing.length > 0 && routingMiddleware) {
destination.addMiddleware(routingMiddleware)
}

Expand Down
129 changes: 125 additions & 4 deletions packages/browser/src/plugins/remote-loader/__tests__/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import * as loader from '../../../lib/load-script'
import { remoteLoader } from '..'
import { AnalyticsBrowser } from '../../../browser'
import { AnalyticsBrowser, LegacySettings } from '../../../browser'
import { InitOptions } from '../../../core/analytics'
import { Context } from '../../../core/context'
import { tsubMiddleware } from '../../routing-middleware'

const pluginFactory = jest.fn()

Expand All @@ -26,6 +28,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remote plugin',
creationName: 'remote plugin',
url: 'cdn/path/to/file.js',
libraryName: 'testPlugin',
settings: {},
Expand All @@ -46,6 +49,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remote plugin',
creationName: 'remote plugin',
url: 'cdn/path/to/file.js',
libraryName: 'testPlugin',
settings: {},
Expand All @@ -71,6 +75,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remote plugin',
creationName: 'remote plugin',
url: 'https://cdn.segment.com/actions/file.js',
libraryName: 'testPlugin',
settings: {},
Expand All @@ -91,6 +96,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remote plugin',
creationName: 'remote plugin',
url: 'cdn/path/to/file.js',
libraryName: 'testPlugin',
settings: {
Expand Down Expand Up @@ -118,6 +124,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remote plugin',
creationName: 'remote plugin',
url: 'cdn/path/to/file.js',
libraryName: 'testPlugin',
settings: {
Expand All @@ -140,6 +147,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remote plugin',
creationName: 'remote plugin',
url: 'cdn/path/to/file.js',
libraryName: 'testPlugin',
settings: {
Expand Down Expand Up @@ -167,6 +175,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remote plugin',
creationName: 'remote plugin',
url: 'cdn/path/to/file.js',
libraryName: 'testPlugin',
settings: {
Expand All @@ -189,6 +198,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remote plugin',
creationName: 'remote plugin',
url: 'cdn/path/to/file.js',
libraryName: 'this wont resolve',
settings: {},
Expand Down Expand Up @@ -245,12 +255,14 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'multiple plugins',
creationName: 'multiple plugins',
url: 'multiple-plugins.js',
libraryName: 'multiple-plugins',
settings: { foo: true },
},
{
name: 'single plugin',
creationName: 'single plugin',
url: 'single-plugin.js',
libraryName: 'single-plugin',
settings: { bar: false },
Expand All @@ -262,7 +274,34 @@ describe('Remote Loader', () => {
)

expect(plugins).toHaveLength(3)
expect(plugins).toEqual(expect.arrayContaining([one, two, three]))
expect(plugins).toEqual(
expect.arrayContaining([
{
action: one,
name: 'multiple plugins',
version: '1.0.0',
type: 'before',
alternativeNames: ['one'],
middleware: [],
},
{
action: two,
name: 'multiple plugins',
version: '1.0.0',
type: 'before',
alternativeNames: ['two'],
middleware: [],
},
{
action: three,
name: 'single plugin',
version: '1.0.0',
type: 'enrichment',
alternativeNames: ['three'],
middleware: [],
},
])
)
expect(multiPluginFactory).toHaveBeenCalledWith({ foo: true })
expect(singlePluginFactory).toHaveBeenCalledWith({ bar: false })
})
Expand All @@ -287,12 +326,14 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'flaky plugin',
creationName: 'flaky plugin',
url: 'cdn/path/to/flaky.js',
libraryName: 'flaky',
settings: {},
},
{
name: 'async flaky plugin',
creationName: 'async flaky plugin',
url: 'cdn/path/to/asyncFlaky.js',
libraryName: 'asyncFlaky',
settings: {},
Expand Down Expand Up @@ -339,12 +380,14 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'valid plugin',
creationName: 'valid plugin',
url: 'valid',
libraryName: 'valid',
settings: { foo: true },
},
{
name: 'invalid plugin',
creationName: 'invalid plugin',
url: 'invalid',
libraryName: 'invalid',
settings: { bar: false },
Expand All @@ -356,12 +399,23 @@ describe('Remote Loader', () => {
)

expect(plugins).toHaveLength(1)
expect(plugins).toEqual(expect.arrayContaining([validPlugin]))
expect(plugins).toEqual(
expect.arrayContaining([
{
action: validPlugin,
name: 'valid plugin',
version: '1.0.0',
type: 'enrichment',
alternativeNames: ['valid'],
middleware: [],
},
])
)
expect(console.warn).toHaveBeenCalledTimes(1)
})

it('accepts settings overrides from merged integrations', async () => {
const cdnSettings = {
const cdnSettings: LegacySettings = {
integrations: {
remotePlugin: {
name: 'Charlie Brown',
Expand All @@ -371,6 +425,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remotePlugin',
creationName: 'remotePlugin',
libraryName: 'testPlugin',
url: 'cdn/path/to/file.js',
settings: {
Expand Down Expand Up @@ -416,6 +471,7 @@ describe('Remote Loader', () => {
remotePlugins: [
{
name: 'remotePlugin',
creationName: 'remotePlugin',
libraryName: 'testPlugin',
url: 'cdn/path/to/file.js',
settings: {
Expand Down Expand Up @@ -452,4 +508,69 @@ describe('Remote Loader', () => {
})
)
})

it('applies remote routing rules based on creation name', async () => {
const validPlugin = {
name: 'valid',
version: '1.0.0',
type: 'enrichment',
load: () => {},
isLoaded: () => true,
track: (ctx: Context) => ctx,
}

const cdnSettings: LegacySettings = {
integrations: {},
middlewareSettings: {
routingRules: [
{
matchers: [
{
ir: '["=","event",{"value":"Item Impression"}]',
type: 'fql',
},
],
scope: 'destinations',
target_type: 'workspace::project::destination::config',
transformers: [[{ type: 'drop' }]],
destinationName: 'oldValidName',
},
],
},
remotePlugins: [
{
name: 'valid',
creationName: 'oldValidName',
url: 'valid',
libraryName: 'valid',
settings: { foo: true },
},
],
}

// @ts-expect-error not gonna return a script tag sorry
jest.spyOn(loader, 'loadScript').mockImplementation((url: string) => {
if (url === 'valid') {
window['valid'] = jest.fn().mockImplementation(() => validPlugin)
}

return Promise.resolve(true)
})

const middleware = tsubMiddleware(
cdnSettings.middlewareSettings!.routingRules
)

const plugins = await remoteLoader(cdnSettings, {}, {}, false, middleware)
const plugin = plugins[0]
await expect(() =>
plugin.track!(new Context({ type: 'track', event: 'Item Impression' }))
).rejects.toMatchInlineSnapshot(`
ContextCancelation {
"reason": "dropped by destination middleware",
"retry": false,
"type": "plugin Error",
}
`)
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ describe.skip('Remote Plugin Integration', () => {
// but I'd like to have a full integration test if possible
{
name: 'amplitude',
creationName: 'amplitude',
url: 'https://ajs-next-integrations.s3-us-west-2.amazonaws.com/fab-5/amplitude-plugins.js',
libraryName: 'amplitude-pluginsDestination',
settings: {
Expand Down
Loading