fix: amplitude.remove should only remove if plugin was already regist…

…ered (#936)
amplitude · Dec 17, 2024 · dd6d235 · dd6d235
2 parents ad319d6 + 37ec7eb
commit dd6d235
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 19 deletions.
diff --git a/packages/analytics-core/src/core-client.ts b/packages/analytics-core/src/core-client.ts
@@ -114,7 +114,7 @@ export class AmplitudeCore implements CoreClient {
   }
 
   _removePlugin(pluginName: string) {
-    return returnWrapper(this.timeline.deregister(pluginName));
+    return returnWrapper(this.timeline.deregister(pluginName, this.config));
   }
 
   dispatchWithCallback(event: Event, callback: (result: Result) => void): void {

diff --git a/packages/analytics-core/src/timeline.ts b/packages/analytics-core/src/timeline.ts
@@ -41,8 +41,12 @@ export class Timeline {
     this.plugins.push(plugin);
   }
 
-  async deregister(pluginName: string) {
+  async deregister(pluginName: string, config: Config) {
     const index = this.plugins.findIndex((plugin) => plugin.name === pluginName);
+    if (index === -1) {
+      config.loggerProvider.warn(`Plugin with name ${pluginName} does not exist, skipping deregistration`);
+      return;
+    }
     const plugin = this.plugins[index];
     this.plugins.splice(index, 1);
     await plugin.teardown?.();

diff --git a/packages/analytics-core/test/timeline.test.ts b/packages/analytics-core/test/timeline.test.ts
@@ -81,6 +81,32 @@ describe('timeline', () => {
     });
   });
 
+  describe('deregister', () => {
+    test('should remove plugin correctly', async () => {
+      await timeline.register(
+        {
+          name: 'test-plugin',
+        },
+        config,
+      );
+      expect(timeline.plugins.length).toBe(1);
+      await timeline.deregister('test-plugin', config);
+      expect(timeline.plugins.length).toBe(0);
+    });
+
+    test('should only remove plugin that was already registered', async () => {
+      await timeline.register(
+        {
+          name: 'test-plugin',
+        },
+        config,
+      );
+      expect(timeline.plugins.length).toBe(1);
+      await timeline.deregister('bad-test-plugin', config);
+      expect(timeline.plugins.length).toBe(1);
+    });
+  });
+
   describe('reset', () => {
     test('should reset timeline', () => {
       const timeline = new Timeline(new AmplitudeCore());
@@ -199,9 +225,9 @@ describe('timeline', () => {
     expect(destinationExecute).toHaveBeenCalledTimes(10);
 
     // deregister
-    await timeline.deregister('plugin:before');
-    await timeline.deregister('plugin:enrichment');
-    await timeline.deregister('plugin:destination');
+    await timeline.deregister('plugin:before', config);
+    await timeline.deregister('plugin:enrichment', config);
+    await timeline.deregister('plugin:destination', config);
     expect(timeline.plugins.length).toBe(0);
   });
 
@@ -233,7 +259,7 @@ describe('timeline', () => {
       };
       await timeline.register(before, config);
       await timeline.apply(undefined);
-      await timeline.deregister('plugin:before');
+      await timeline.deregister('plugin:before', config);
       expect(beforeExecute).toHaveBeenCalledTimes(0);
     });
 
@@ -282,9 +308,9 @@ describe('timeline', () => {
       const callback = jest.fn();
       await timeline.apply([event, callback]);
 
-      await timeline.deregister('plugin:before');
-      await timeline.deregister('plugin:enrichment');
-      await timeline.deregister('plugin:destination');
+      await timeline.deregister('plugin:before', config);
+      await timeline.deregister('plugin:enrichment', config);
+      await timeline.deregister('plugin:destination', config);
 
       expect(beforeExecute).toHaveBeenCalledTimes(1);
       expect(enrichmentExecute).toHaveBeenCalledTimes(1);
@@ -353,11 +379,11 @@ describe('timeline', () => {
       const callback = jest.fn();
       await timeline.apply([event, callback]);
 
-      await timeline.deregister('plugin:before:1');
-      await timeline.deregister('plugin:before:2');
-      await timeline.deregister('plugin:before:3');
-      await timeline.deregister('plugin:enrichment');
-      await timeline.deregister('plugin:destination');
+      await timeline.deregister('plugin:before:1', config);
+      await timeline.deregister('plugin:before:2', config);
+      await timeline.deregister('plugin:before:3', config);
+      await timeline.deregister('plugin:enrichment', config);
+      await timeline.deregister('plugin:destination', config);
 
       expect(beforeExecute1).toHaveBeenCalledTimes(1);
       expect(beforeExecute2).toHaveBeenCalledTimes(1);
@@ -428,11 +454,11 @@ describe('timeline', () => {
       const callback = jest.fn();
       await timeline.apply([event, callback]);
 
-      await timeline.deregister('plugin:before');
-      await timeline.deregister('plugin:enrichment:1');
-      await timeline.deregister('plugin:enrichment:2');
-      await timeline.deregister('plugin:enrichment:3');
-      await timeline.deregister('plugin:destination');
+      await timeline.deregister('plugin:before', config);
+      await timeline.deregister('plugin:enrichment:1', config);
+      await timeline.deregister('plugin:enrichment:2', config);
+      await timeline.deregister('plugin:enrichment:3', config);
+      await timeline.deregister('plugin:destination', config);
 
       expect(beforeExecute).toHaveBeenCalledTimes(1);
       expect(enrichmentExecute1).toHaveBeenCalledTimes(1);