Skip to content

Commit

Permalink
fix(core): Do not run setup for integration on client multiple times (
Browse files Browse the repository at this point in the history 
#10116)

Currently, if you do:

```js
const myIntegration = new InboundFilters();
const myIntegration2 = new InboundFilters();
const myIntegration3 = new InboundFilters();

client.addIntegration(myIntegration);
client.addIntegration(myIntegration2);
client.addIntegration(myIntegration3);
```

All of these will have their `setup` hooks called, and thus they will be
initialized multiple times. However, all but the last will be discarded
from the client and not be accessible anymore via e.g.
`getIntegration()` or similar.

This used to not matter because everything was guarded through
`setupOnce()` anyhow, but i would say this is more of a bug and not
really expected.

With this change, an integration can only be added to a client once, if
you try to add it again it will noop.
  • Loading branch information
@mydea
mydea authored Jan 9, 2024
1 parent f70128d commit 15ab8ee
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 39 deletions.
4 changes: 4 additions & 0 deletions packages/core/src/integration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ export function setupIntegrations(client: Client, integrations: Integration[]):

/** Setup a single integration. */
export function setupIntegration(client: Client, integration: Integration, integrationIndex: IntegrationIndex): void {
if (integrationIndex[integration.name]) {
DEBUG_BUILD && logger.log(`Integration skipped because it was already installed: ${integration.name}`);
return;
}
integrationIndex[integration.name] = integration;

// `setupOnce` is only called the first time
Expand Down
70 changes: 31 additions & 39 deletions packages/core/test/lib/integration.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ describe('setupIntegration', () => {
setupIntegration(client2, integration3, integrationIndex);
setupIntegration(client2, integration4, integrationIndex);

expect(integrationIndex).toEqual({ test: integration4 });
expect(integrationIndex).toEqual({ test: integration1 });
expect(integration1.setupOnce).toHaveBeenCalledTimes(1);
expect(integration2.setupOnce).not.toHaveBeenCalled();
expect(integration3.setupOnce).not.toHaveBeenCalled();
Expand All @@ -394,32 +394,32 @@ describe('setupIntegration', () => {
const client1 = getTestClient();
const client2 = getTestClient();

const integrationIndex = {};
const integrationIndex1 = {};
const integrationIndex2 = {};
const integration1 = new CustomIntegration();
const integration2 = new CustomIntegration();
const integration3 = new CustomIntegration();
const integration4 = new CustomIntegration();

setupIntegration(client1, integration1, integrationIndex);
setupIntegration(client1, integration2, integrationIndex);
setupIntegration(client2, integration3, integrationIndex);
setupIntegration(client2, integration4, integrationIndex);
setupIntegration(client1, integration1, integrationIndex1);
setupIntegration(client1, integration2, integrationIndex1);
setupIntegration(client2, integration3, integrationIndex2);
setupIntegration(client2, integration4, integrationIndex2);

expect(integrationIndex).toEqual({ test: integration4 });
expect(integrationIndex1).toEqual({ test: integration1 });
expect(integrationIndex2).toEqual({ test: integration3 });
expect(integration1.setupOnce).toHaveBeenCalledTimes(1);
expect(integration2.setupOnce).not.toHaveBeenCalled();
expect(integration3.setupOnce).not.toHaveBeenCalled();
expect(integration4.setupOnce).not.toHaveBeenCalled();

expect(integration1.setup).toHaveBeenCalledTimes(1);
expect(integration2.setup).toHaveBeenCalledTimes(1);
expect(integration2.setup).toHaveBeenCalledTimes(0);
expect(integration3.setup).toHaveBeenCalledTimes(1);
expect(integration4.setup).toHaveBeenCalledTimes(1);
expect(integration4.setup).toHaveBeenCalledTimes(0);

expect(integration1.setup).toHaveBeenCalledWith(client1);
expect(integration2.setup).toHaveBeenCalledWith(client1);
expect(integration3.setup).toHaveBeenCalledWith(client2);
expect(integration4.setup).toHaveBeenCalledWith(client2);
});

it('binds preprocessEvent for each client', () => {
Expand All @@ -432,18 +432,20 @@ describe('setupIntegration', () => {
const client1 = getTestClient();
const client2 = getTestClient();

const integrationIndex = {};
const integrationIndex1 = {};
const integrationIndex2 = {};
const integration1 = new CustomIntegration();
const integration2 = new CustomIntegration();
const integration3 = new CustomIntegration();
const integration4 = new CustomIntegration();

setupIntegration(client1, integration1, integrationIndex);
setupIntegration(client1, integration2, integrationIndex);
setupIntegration(client2, integration3, integrationIndex);
setupIntegration(client2, integration4, integrationIndex);
setupIntegration(client1, integration1, integrationIndex1);
setupIntegration(client1, integration2, integrationIndex1);
setupIntegration(client2, integration3, integrationIndex2);
setupIntegration(client2, integration4, integrationIndex2);

expect(integrationIndex).toEqual({ test: integration4 });
expect(integrationIndex1).toEqual({ test: integration1 });
expect(integrationIndex2).toEqual({ test: integration3 });
expect(integration1.setupOnce).toHaveBeenCalledTimes(1);
expect(integration2.setupOnce).not.toHaveBeenCalled();
expect(integration3.setupOnce).not.toHaveBeenCalled();
Expand All @@ -456,14 +458,12 @@ describe('setupIntegration', () => {
client2.captureEvent({ event_id: '2c' });

expect(integration1.preprocessEvent).toHaveBeenCalledTimes(2);
expect(integration2.preprocessEvent).toHaveBeenCalledTimes(2);
expect(integration2.preprocessEvent).toHaveBeenCalledTimes(0);
expect(integration3.preprocessEvent).toHaveBeenCalledTimes(3);
expect(integration4.preprocessEvent).toHaveBeenCalledTimes(3);
expect(integration4.preprocessEvent).toHaveBeenCalledTimes(0);

expect(integration1.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '1b' }, {}, client1);
expect(integration2.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '1b' }, {}, client1);
expect(integration3.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '2c' }, {}, client2);
expect(integration4.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '2c' }, {}, client2);
});

it('allows to mutate events in preprocessEvent', async () => {
Expand Down Expand Up @@ -504,18 +504,20 @@ describe('setupIntegration', () => {
const client1 = getTestClient();
const client2 = getTestClient();

const integrationIndex = {};
const integrationIndex1 = {};
const integrationIndex2 = {};
const integration1 = new CustomIntegration();
const integration2 = new CustomIntegration();
const integration3 = new CustomIntegration();
const integration4 = new CustomIntegration();

setupIntegration(client1, integration1, integrationIndex);
setupIntegration(client1, integration2, integrationIndex);
setupIntegration(client2, integration3, integrationIndex);
setupIntegration(client2, integration4, integrationIndex);
setupIntegration(client1, integration1, integrationIndex1);
setupIntegration(client1, integration2, integrationIndex1);
setupIntegration(client2, integration3, integrationIndex2);
setupIntegration(client2, integration4, integrationIndex2);

expect(integrationIndex).toEqual({ test: integration4 });
expect(integrationIndex1).toEqual({ test: integration1 });
expect(integrationIndex2).toEqual({ test: integration3 });
expect(integration1.setupOnce).toHaveBeenCalledTimes(1);
expect(integration2.setupOnce).not.toHaveBeenCalled();
expect(integration3.setupOnce).not.toHaveBeenCalled();
Expand All @@ -528,30 +530,20 @@ describe('setupIntegration', () => {
client2.captureEvent({ event_id: '2c' });

expect(integration1.processEvent).toHaveBeenCalledTimes(2);
expect(integration2.processEvent).toHaveBeenCalledTimes(2);
expect(integration2.processEvent).toHaveBeenCalledTimes(0);
expect(integration3.processEvent).toHaveBeenCalledTimes(3);
expect(integration4.processEvent).toHaveBeenCalledTimes(3);
expect(integration4.processEvent).toHaveBeenCalledTimes(0);

expect(integration1.processEvent).toHaveBeenLastCalledWith(
expect.objectContaining({ event_id: '1b' }),
{},
client1,
);
expect(integration2.processEvent).toHaveBeenLastCalledWith(
expect.objectContaining({ event_id: '1b' }),
{},
client1,
);
expect(integration3.processEvent).toHaveBeenLastCalledWith(
expect.objectContaining({ event_id: '2c' }),
{},
client2,
);
expect(integration4.processEvent).toHaveBeenLastCalledWith(
expect.objectContaining({ event_id: '2c' }),
{},
client2,
);
});

it('allows to mutate events in processEvent', async () => {
Expand Down

0 comments on commit 15ab8ee

Please sign in to comment.