fix: stop leaking scroll listeners under React StrictMode

src/components/Redoc/Redoc.tsx

@@ -28,6 +28,14 @@ export class Redoc extends React.Component<RedocProps> {
     this.props.store.onDidMount();
   }
 
+  componentDidUpdate(prevProps: RedocProps) {
+    if (prevProps.store !== this.props.store) {
+      // only detach: the previous store may be passed in again, so its search worker must keep running
+      prevProps.store.detach();
+      this.props.store.onDidMount();
+    }
+  }
+
   componentWillUnmount() {
     this.props.store.dispose();
   }

src/components/__tests__/Redoc.test.tsx

@@ -0,0 +1,138 @@
+/* tslint:disable:no-implicit-dependencies */
+import * as React from 'react';
+import { createRoot, Root } from 'react-dom/client';
+import { act } from 'react-dom/test-utils';
+
+import { Redoc, RedocStandalone } from '../';
+import { AppStore } from '../../services';
+
+// jest falls back to the in-process search worker, which has no terminate()
+const options = { disableSearch: true };
+
+const spec = {
+  openapi: '3.0.0',
+  info: { title: 'Test', version: '1.0.0' },
+  paths: {
+    '/pets': {
+      get: { operationId: 'listPets', responses: { '200': { description: 'OK' } } },
+    },
+  },
+};
+
+describe('Redoc scroll listeners', () => {
+  const scrollListeners = new Set<EventListenerOrEventListenerObject>();
+  let container: HTMLDivElement;
+  let root: Root;
+
+  beforeAll(() => {
+    (globalThis as any).IS_REACT_ACT_ENVIRONMENT = true;
+  });
+
+  beforeEach(() => {
+    scrollListeners.clear();
+    const addEventListener = window.addEventListener;
+    const removeEventListener = window.removeEventListener;
+    jest.spyOn(window, 'addEventListener').mockImplementation((type, listener, options) => {
+      if (type === 'scroll') scrollListeners.add(listener);
+      addEventListener.call(window, type, listener, options);
+    });
+    jest.spyOn(window, 'removeEventListener').mockImplementation((type, listener, options) => {
+      if (type === 'scroll') scrollListeners.delete(listener);
+      removeEventListener.call(window, type, listener, options);
+    });
+    container = document.createElement('div');
+    document.body.appendChild(container);
+    root = createRoot(container);
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+    container.remove();
+  });
+
+  const scrollSubscribers = (store: AppStore) =>
+    (store as any).scroll._emiter.listenerCount('scroll') as number;
+
+  test('RedocStandalone removes every scroll listener on unmount in StrictMode', async () => {
+    let loaded!: () => void;
+    const onLoaded = new Promise<void>(resolve => (loaded = resolve));
+
+    await act(async () => {
+      root.render(
+        <React.StrictMode>
+          <RedocStandalone
+            spec={spec}
+            options={{ ...options, scrollYOffset: 50 }}
+            onLoaded={() => loaded()}
+          />
+        </React.StrictMode>,
+      );
+    });
+    await act(async () => {
+      await onLoaded;
+    });
+
+    expect(scrollListeners.size).toBe(1);
+
+    act(() => root.unmount());
+
+    expect(scrollListeners.size).toBe(0);
+  });
+
+  test('Redoc keeps the scroll-spy of its store after a StrictMode remount', () => {
+    const store = new AppStore(spec as any, undefined, options);
+
+    act(() => {
+      root.render(
+        <React.StrictMode>
+          <Redoc store={store} />
+        </React.StrictMode>,
+      );
+    });
+
+    expect(scrollListeners.size).toBe(1);
+    expect(scrollSubscribers(store)).toBe(1);
+
+    act(() => root.unmount());
+
+    expect(scrollListeners.size).toBe(0);
+  });
+
+  test('Redoc detaches the previous store when it receives a new one', () => {
+    const first = new AppStore(spec as any, undefined, options);
+    const second = new AppStore(spec as any, undefined, options);
+
+    act(() => root.render(<Redoc store={first} />));
+    act(() => root.render(<Redoc store={second} />));
+
+    expect(scrollListeners.size).toBe(1);
+    expect(scrollSubscribers(first)).toBe(0);
+    expect(scrollSubscribers(second)).toBe(1);
+
+    act(() => root.unmount());
+
+    expect(scrollListeners.size).toBe(0);
+  });
+
+  test('Redoc keeps the search worker of a store that is passed in again', () => {
+    const first = new AppStore(spec as any);
+    const second = new AppStore(spec as any);
+    // the in-process worker used by jest has no terminate()
+    const terminateFirst = jest.fn();
+    (first.search!.searchWorker as any).terminate = terminateFirst;
+    (second.search!.searchWorker as any).terminate = jest.fn();
+
+    act(() => root.render(<Redoc store={first} />));
+    act(() => root.render(<Redoc store={second} />));
+    act(() => root.render(<Redoc store={first} />));
+
+    expect(terminateFirst).not.toHaveBeenCalled();
+    expect(scrollSubscribers(first)).toBe(1);
+    expect(scrollSubscribers(second)).toBe(0);
+
+    act(() => root.unmount());
+
+    expect(terminateFirst).toHaveBeenCalledTimes(1);
+    expect(scrollListeners.size).toBe(0);
+  });
+});

src/services/AppStore.ts

@@ -79,28 +79,50 @@ export class AppStore {
         this.search.indexItems(this.menu.items);
       }
 
-      this.disposer = observe(this.menu, 'activeItemIdx', change => {
-        this.updateMarkOnMenu(change.newValue as number);
-      });
+      this.observeActiveItem();
     }
   }
 
   onDidMount() {
+    // listeners are attached on mount (not in the constructor), so a store that is never
+    // mounted, e.g. one built twice by React StrictMode, doesn't leak them, and a store that
+    // is disposed and mounted again (StrictMode remount) gets them back
+    this.scroll.bind();
+    this.menu.subscribe();
+    if (this.search) {
+      this.observeActiveItem();
+    }
     this.menu.updateOnHistory();
     this.updateMarkOnMenu(this.menu.activeItemIdx);
   }
 
-  dispose() {
+  /**
+   * Removes the scroll, history and menu listeners attached by `onDidMount`.
+   * The store stays usable and can be mounted again; the search worker keeps running.
+   */
+  detach() {
     this.scroll.dispose();
     this.menu.dispose();
-    if (this.search) {
-      this.search.dispose();
-    }
     if (this.disposer != null) {
       this.disposer();
+      this.disposer = null;
     }
   }
 
+  dispose() {
+    this.detach();
+    if (this.search) {
+      this.search.dispose();
+    }
+  }
+
+  private observeActiveItem() {
+    if (this.disposer != null) return;
+    this.disposer = observe(this.menu, 'activeItemIdx', change => {
+      this.updateMarkOnMenu(change.newValue as number);
+    });
+  }
+
   /**
    * serializes store
    * **SUPER HACKY AND NOT OPTIMAL IMPLEMENTATION**

src/services/MenuStore.ts

@@ -46,8 +46,8 @@ export class MenuStore {
   /**
    * cached flattened menu items to support absolute indexing
    */
-  private _unsubscribe: () => void;
-  private _hashUnsubscribe: () => void;
+  private _unsubscribe?: () => void;
+  private _hashUnsubscribe?: () => void;
 
   /**
    *
@@ -61,13 +61,18 @@ export class MenuStore {
 
     this.flatItems = flattenByProp(this.items || [], 'items');
     this.flatItems.forEach((item, idx) => (item.absoluteIdx = idx));
-
-    this.subscribe();
   }
 
+  /**
+   * Subscribes to scroll and history updates. Called when the store is mounted.
+   */
   subscribe() {
-    this._unsubscribe = this.scroll.subscribe(this.updateOnScroll);
-    this._hashUnsubscribe = this.history.subscribe(this.updateOnHistory);
+    if (!this._unsubscribe) {
+      this._unsubscribe = this.scroll.subscribe(this.updateOnScroll);
+    }
+    if (!this._hashUnsubscribe) {
+      this._hashUnsubscribe = this.history.subscribe(this.updateOnHistory);
+    }
   }
 
   @action
@@ -252,7 +257,9 @@ export class MenuStore {
   }
 
   dispose() {
-    this._unsubscribe();
-    this._hashUnsubscribe();
+    this._unsubscribe?.();
+    this._hashUnsubscribe?.();
+    this._unsubscribe = undefined;
+    this._hashUnsubscribe = undefined;
   }
 }

src/services/ScrollService.ts

@@ -10,20 +10,27 @@ export class ScrollService {
   private _scrollParent: Window | HTMLElement | undefined;
   private _emiter: EventEmitter;
   private _prevOffsetY = 0;
+  private _bound = false;
   constructor(private options: RedocNormalizedOptions) {
     this._scrollParent = IS_BROWSER ? window : undefined;
     this._emiter = new EventEmitter();
-    this.bind();
   }
 
+  /**
+   * Starts listening to scroll events. Called when the store is mounted, so a store
+   * that is never mounted does not leave a listener behind.
+   */
   bind() {
+    if (this._bound) return;
+    this._bound = true;
     this._prevOffsetY = this.scrollY();
     if (this._scrollParent) {
       this._scrollParent.addEventListener('scroll', this.handleScroll);
     }
   }
 
   dispose() {
+    this._bound = false;
     if (this._scrollParent) {
       this._scrollParent.removeEventListener('scroll', this.handleScroll);
     }