Skip to content

Commit

Permalink
Fix action destination middleware bug (#629)
Browse files Browse the repository at this point in the history
* only transform destinations

* add changeset

* Add another test around mutating context

* Update comment

* Only add middleware to destination types
  • Loading branch information
danieljackins authored Oct 18, 2022
1 parent 6c35799 commit 21f05ad
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 3 deletions.
5 changes: 5 additions & 0 deletions .changeset/rich-icons-nail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-next': patch
---

Fix bug where destination middleware were applying to other plugin types
103 changes: 102 additions & 1 deletion packages/browser/src/plugins/remote-loader/__tests__/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as loader from '../../../lib/load-script'
import { remoteLoader } from '..'
import { ActionDestination, remoteLoader } from '..'
import { AnalyticsBrowser, LegacySettings } from '../../../browser'
import { InitOptions } from '../../../core/analytics'
import { Context } from '../../../core/context'
Expand Down Expand Up @@ -597,4 +597,105 @@ describe('Remote Loader', () => {
}
`)
})

it('only applies destination middleware to destination actions', 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 = jest.fn().mockImplementation(() => true)

const plugins = await remoteLoader(cdnSettings, {}, {}, false, middleware)
const plugin = plugins[0] as ActionDestination
plugin.addMiddleware(middleware)
await plugin.track(new Context({ type: 'track' }))
expect(middleware).not.toHaveBeenCalled()
})

it('non destination type plugins can modify the context', async () => {
const validPlugin = {
name: 'valid',
version: '1.0.0',
type: 'enrichment',
load: () => {},
isLoaded: () => true,
track: (ctx: Context) => {
ctx.event.name += 'bar'
return ctx
},
}

const cdnSettings: LegacySettings = {
integrations: {},
remotePlugins: [
{
name: 'valid',
creationName: 'valid',
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 plugins = await remoteLoader(cdnSettings, {}, {}, false)
const plugin = plugins[0] as ActionDestination
const newCtx = await plugin.track(
new Context({ type: 'track', name: 'foo' })
)

expect(newCtx.event.name).toEqual('foobar')
})
})
11 changes: 9 additions & 2 deletions packages/browser/src/plugins/remote-loader/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ export class ActionDestination implements Plugin {
}

addMiddleware(...fn: DestinationMiddlewareFunction[]): void {
this.middleware.push(...fn)
if (this.type === 'destination') {
this.middleware.push(...fn)
}
}

private async transform(ctx: Context): Promise<Context> {
Expand Down Expand Up @@ -72,7 +74,12 @@ export class ActionDestination implements Plugin {
return async (ctx: Context): Promise<Context> => {
if (!this.action[methodName]) return ctx

const transformedContext = await this.transform(ctx)
let transformedContext: Context = ctx
// Transformations only allowed for destination plugins. Other plugin types support mutating events.
if (this.type === 'destination') {
transformedContext = await this.transform(ctx)
}

await this.action[methodName]!(transformedContext)

return ctx
Expand Down

0 comments on commit 21f05ad

Please sign in to comment.