fix(global-listeners): doesn't unsubscribe all listener unexpectedly

sandpack-client/src/iframe-protocol.ts

@@ -13,7 +13,7 @@ export class IFrameProtocol {
   private globalListenersCount = 0;
 
   // React to messages from the iframe owned by this instance
-  private channelListeners: Record<number, ListenerFunction> = {};
+  public channelListeners: Record<number, ListenerFunction> = {};
   private channelListenersCount = 0;
 
   // Random number to identify this instance of the client when messages are coming from multiple iframes

sandpack-react/src/contexts/sandpackContext.test.tsx

@@ -1,24 +1,47 @@
+/**
+ * @jest-environment jsdom
+ */
 import React from "react";
 import { create } from "react-test-renderer";
 
 import { REACT_TEMPLATE } from "..";
 
+import type { SandpackProviderClass } from "./sandpackContext";
 import { SandpackProvider } from "./sandpackContext";
 
+const createContext = (): SandpackProviderClass => {
+  const root = create(<SandpackProvider template="react" />);
+  const instance = root.getInstance();
+
+  instance.runSandpack();
+
+  return instance;
+};
+
+const getAmountOfListener = (
+  instance: SandpackProviderClass,
+  name = "client-id",
+  ignoreGlobalListener = false
+): number => {
+  return (
+    Object.keys(instance.clients[name].iframeProtocol.channelListeners).length -
+    1 - // less protocol listener
+    (ignoreGlobalListener ? 0 : 1) // less the global Sandpack-react listener
+  );
+};
+
 describe(SandpackProvider, () => {
   describe("updateFile", () => {
     it("adds a file", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
 
       instance.addFile({ "new-file.js": "new-content" });
 
       expect(instance.state.files["new-file.js"].code).toBe("new-content");
     });
 
     it("deletes a file", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
 
       instance.deleteFile("/App.js");
 
@@ -32,8 +55,7 @@ describe(SandpackProvider, () => {
     });
 
     it("updates a file", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
 
       expect(instance.state.files["/App.js"]).toEqual({
         code: `export default function App() {
@@ -48,8 +70,7 @@ describe(SandpackProvider, () => {
     });
 
     it("updates multiples files", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
 
       instance.updateFile({ "/App.js": "Foo", "/index.js": "Baz" });
 
@@ -58,8 +79,7 @@ describe(SandpackProvider, () => {
     });
 
     it("updates multiples files in a row", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
 
       instance.updateFile("/App.js", "Foo");
       instance.updateFile("/index.js", "Baz");
@@ -71,15 +91,13 @@ describe(SandpackProvider, () => {
 
   describe("editorState", () => {
     it("should return the same initial state", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
 
       expect(instance.state.editorState).toBe("pristine");
     });
 
     it("should return a dirty value after updating a file", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
 
       expect(instance.state.editorState).toBe("pristine");
 
@@ -88,8 +106,7 @@ describe(SandpackProvider, () => {
     });
 
     it("should return a pristine value after reseting files", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
 
       expect(instance.state.editorState).toBe("pristine");
 
@@ -101,8 +118,7 @@ describe(SandpackProvider, () => {
     });
 
     it("should return a pristine value after reverting a change", () => {
-      const root = create(<SandpackProvider template="react" />);
-      const instance = root.getInstance();
+      const instance = createContext();
       expect(instance.state.editorState).toBe("pristine");
 
       instance.updateFile("/App.js", "Foo");
@@ -113,4 +129,208 @@ describe(SandpackProvider, () => {
       expect(instance.state.editorState).toBe("pristine");
     });
   });
+
+  describe("listeners", () => {
+    it("sets a listener, but the client hasn't been created yet - no global listener", () => {
+      const instance = createContext();
+
+      // Act: Add listener
+      const mock = jest.fn();
+      instance.addListener(mock, "client-id");
+
+      // Act: Create client
+      instance.registerBundler(document.createElement("iframe"), "client-id");
+
+      // Expect: one pending unsubscribe function
+      expect(
+        Object.keys(instance.unsubscribeClientListeners["client-id"]).length
+      ).toBe(1);
+
+      // Expect: no global listener
+      expect(Object.keys(instance.queuedListeners.global).length).toBe(0);
+
+      // Expect: one client
+      expect(Object.keys(instance.clients)).toEqual(["client-id"]);
+
+      /**
+       * TODO: figure out how to mock SandpackClient and invoke the listener func
+       */
+      // expect(mock).toHaveBeenCalled();
+    });
+
+    it("sets a listener, but the client hasn't been created yet - global listener", () => {
+      const instance = createContext();
+
+      // Act: Add listener
+      const mock = jest.fn();
+      instance.addListener(mock /* , no client-id */);
+
+      // Act: Create client
+      instance.registerBundler(document.createElement("iframe"), "client-id");
+
+      // Expect: one pending unsubscribe function
+      expect(
+        Object.keys(instance.unsubscribeClientListeners["client-id"]).length
+      ).toBe(1);
+
+      // Expect: no global listener
+      expect(Object.keys(instance.queuedListeners.global).length).toBe(1);
+
+      // Expect: one listener in the client
+      expect(getAmountOfListener(instance)).toBe(1);
+    });
+
+    it("set a listener, but the client has already been created - no global listener", () => {
+      const instance = createContext();
+
+      // Act: Create client
+      instance.registerBundler(document.createElement("iframe"), "client-id");
+
+      // Expect: no pending unsubscribe function
+      expect(
+        Object.keys(instance.unsubscribeClientListeners["client-id"]).length
+      ).toBe(0);
+
+      // Expect: no global listener
+      expect(Object.keys(instance.queuedListeners.global).length).toBe(0);
+
+      // Act: Add listener
+      const mock = jest.fn();
+      instance.addListener(mock, "client-id");
+
+      // Expect: no pending unsubscribe function
+      expect(
+        Object.keys(instance.unsubscribeClientListeners["client-id"]).length
+      ).toBe(0);
+
+      // Expect: no global listener
+      expect(Object.keys(instance.queuedListeners.global).length).toBe(0);
+
+      // Expect: one listener in the client
+      expect(getAmountOfListener(instance)).toBe(1);
+    });
+
+    it("set a listener, but the client has already been created - global listener", () => {
+      const instance = createContext();
+
+      // Act: Create client
+      instance.registerBundler(document.createElement("iframe"), "client-id");
+
+      // Expect: no pending unsubscribe function
+      expect(
+        Object.keys(instance.unsubscribeClientListeners["client-id"]).length
+      ).toBe(0);
+
+      // Expect: no global listener
+      expect(Object.keys(instance.queuedListeners.global).length).toBe(0);
+
+      // Act: Add listener
+      const mock = jest.fn();
+      instance.addListener(mock /* , no client-id */);
+
+      // Expect: no pending unsubscribe function, because it's a global
+      expect(
+        Object.keys(instance.unsubscribeClientListeners["client-id"]).length
+      ).toBe(0);
+
+      // Expect: one global listener
+      expect(Object.keys(instance.queuedListeners.global).length).toBe(1);
+
+      // Expect: one listener in the client
+      expect(getAmountOfListener(instance)).toBe(1);
+    });
+
+    it("sets a new listener, and then create one more client", () => {
+      const instance = createContext();
+
+      // Act: Add listener
+      const mock = jest.fn();
+      instance.addListener(mock, "client-id");
+
+      // Act: Create client
+      instance.registerBundler(document.createElement("iframe"), "client-id");
+
+      // Expect: one pending unsubscribe function
+      expect(
+        Object.keys(instance.unsubscribeClientListeners["client-id"]).length
+      ).toBe(1);
+
+      // Expect: no global listener
+      expect(Object.keys(instance.queuedListeners.global).length).toBe(0);
+
+      // Expect: one listener in the client
+      expect(getAmountOfListener(instance)).toBe(1);
+
+      // Act: Add one more listener
+      const anotherMock = jest.fn();
+      instance.addListener(anotherMock /* , no client-id */);
+
+      // Expect: one global listener
+      expect(Object.keys(instance.queuedListeners.global).length).toBe(1);
+
+      // Expect: two listener in the client
+      expect(getAmountOfListener(instance)).toBe(2);
+    });
+
+    it("unsubscribes only from the assigned client id", () => {
+      const instance = createContext();
+
+      instance.registerBundler(document.createElement("iframe"), "client-1");
+      instance.registerBundler(document.createElement("iframe"), "client-2");
+
+      // Initial state
+      expect(getAmountOfListener(instance, "client-1")).toBe(0);
+      expect(getAmountOfListener(instance, "client-2", true)).toBe(0);
+
+      // Add listeners
+      instance.addListener(jest.fn(), "client-1");
+      const unsubscribeClientTwo = instance.addListener(jest.fn(), "client-2");
+
+      expect(getAmountOfListener(instance, "client-1")).toBe(1);
+      expect(getAmountOfListener(instance, "client-2", true)).toBe(1);
+
+      unsubscribeClientTwo();
+
+      expect(getAmountOfListener(instance, "client-1")).toBe(1);
+      expect(getAmountOfListener(instance, "client-2", true)).toBe(0);
+    });
+
+    it("doesn't trigger global unsubscribe", () => {
+      const instance = createContext();
+
+      instance.registerBundler(document.createElement("iframe"), "client-1");
+      instance.registerBundler(document.createElement("iframe"), "client-2");
+
+      instance.addListener(jest.fn());
+      instance.addListener(jest.fn());
+      const unsubscribe = instance.addListener(jest.fn());
+
+      expect(getAmountOfListener(instance, "client-1")).toBe(3);
+      expect(getAmountOfListener(instance, "client-2", true)).toBe(3);
+
+      unsubscribe();
+
+      expect(getAmountOfListener(instance, "client-1")).toBe(2);
+      expect(getAmountOfListener(instance, "client-2", true)).toBe(2);
+    });
+
+    it("unsubscribe all the listeners from a specific client when it unmonts", () => {
+      const instance = createContext();
+
+      instance.registerBundler(document.createElement("iframe"), "client-1");
+      instance.registerBundler(document.createElement("iframe"), "client-2");
+
+      instance.addListener(jest.fn());
+      instance.addListener(jest.fn());
+      instance.addListener(jest.fn());
+
+      expect(getAmountOfListener(instance, "client-1")).toBe(3);
+      expect(getAmountOfListener(instance, "client-2", true)).toBe(3);
+
+      instance.unregisterBundler("client-2");
+
+      expect(getAmountOfListener(instance, "client-1")).toBe(3);
+      expect(instance.clients["client-2"]).toBe(undefined);
+    });
+  });
 });