fix: PageCollector subscribing multiple times (#241)

We were setting the storage inside the first collect callback which mean
that we double subscribed to the events.

This affected Network request and Console messages

Author: Lightning00Blade

src/PageCollector.ts

@@ -9,6 +9,11 @@ import type {Browser, HTTPRequest, Page} from 'puppeteer-core';
 export class PageCollector<T> {
   #browser: Browser;
   #initializer: (page: Page, collector: (item: T) => void) => void;
+  /**
+   * The Array in this map should only be set once
+   * As we use the reference to it.
+   * Use methods that manipulate the array in place.
+   */
   protected storage = new WeakMap<Page, T[]>();
 
   constructor(
@@ -30,7 +35,6 @@ export class PageCollector<T> {
       if (!page) {
         return;
       }
-
       this.#initializePage(page);
     });
   }
@@ -44,6 +48,9 @@ export class PageCollector<T> {
       return;
     }
 
+    const stored: T[] = [];
+    this.storage.set(page, stored);
+
     page.on('framenavigated', frame => {
       // Only reset the storage on main frame navigation
       if (frame !== page.mainFrame()) {
@@ -52,16 +59,16 @@ export class PageCollector<T> {
       this.cleanup(page);
     });
     this.#initializer(page, value => {
-      const stored = this.storage.get(page) ?? [];
       stored.push(value);
-      this.storage.set(page, stored);
     });
   }
 
   protected cleanup(page: Page) {
-    const collection = this.storage.get(page) ?? [];
-    // Keep the reference alive
-    collection.length = 0;
+    const collection = this.storage.get(page);
+    if (collection) {
+      // Keep the reference alive
+      collection.length = 0;
+    }
   }
 
   getData(page: Page): T[] {
@@ -72,13 +79,17 @@ export class PageCollector<T> {
 export class NetworkCollector extends PageCollector<HTTPRequest> {
   override cleanup(page: Page) {
     const requests = this.storage.get(page) ?? [];
+    if (!requests) {
+      return;
+    }
     const lastRequestIdx = requests.findLastIndex(request => {
       return request.frame() === page.mainFrame()
         ? request.isNavigationRequest()
         : false;
     });
     // Keep all requests since the last navigation request including that
     // navigation request itself.
-    this.storage.set(page, requests.slice(Math.max(lastRequestIdx, 0)));
+    // Keep the reference
+    requests.splice(0, Math.max(lastRequestIdx, 0));
   }
 }

tests/PageCollector.test.ts

@@ -6,25 +6,37 @@
 import assert from 'node:assert';
 import {describe, it} from 'node:test';
 
-import type {Browser, Frame, Page, PageEvents} from 'puppeteer-core';
+import type {Browser, Frame, Page, Target} from 'puppeteer-core';
 
 import {PageCollector} from '../src/PageCollector.js';
 
 import {getMockRequest} from './utils.js';
 
-function getMockPage(): Page {
-  const listeners: Record<keyof PageEvents, (data: unknown) => void> = {};
-  const mainFrame = {} as Frame;
+function mockListener() {
+  const listeners: Record<string, Array<(data: unknown) => void>> = {};
   return {
-    on(eventName, listener) {
-      listeners[eventName] = listener;
+    on(eventName: string, listener: (data: unknown) => void) {
+      if (listeners[eventName]) {
+        listeners[eventName].push(listener);
+      } else {
+        listeners[eventName] = [listener];
+      }
     },
-    emit(eventName, data) {
-      listeners[eventName]?.(data);
+    emit(eventName: string, data: unknown) {
+      for (const listener of listeners[eventName] ?? []) {
+        listener(data);
+      }
     },
+  };
+}
+
+function getMockPage(): Page {
+  const mainFrame = {} as Frame;
+  return {
     mainFrame() {
       return mainFrame;
     },
+    ...mockListener(),
   } as Page;
 }
 
@@ -34,9 +46,7 @@ function getMockBrowser(): Browser {
     pages() {
       return Promise.resolve(pages);
     },
-    on(_type, _handler) {
-      // Mock
-    },
+    ...mockListener(),
   } as Browser;
 }
 
@@ -113,4 +123,34 @@ describe('PageCollector', () => {
 
     assert.equal(collector.getData(page).length, 1);
   });
+
+  it('should only subscribe once', async () => {
+    const browser = getMockBrowser();
+    const page = (await browser.pages())[0];
+    const request = getMockRequest();
+    const collector = new PageCollector(browser, (pageListener, collect) => {
+      pageListener.on('request', req => {
+        collect(req);
+      });
+    });
+    await collector.init();
+    browser.emit('targetcreated', {
+      page() {
+        return Promise.resolve(page);
+      },
+    } as Target);
+
+    // The page inside part is async so we need to await some time
+    await new Promise<void>(res => res());
+
+    assert.equal(collector.getData(page).length, 0);
+
+    page.emit('request', request);
+
+    assert.equal(collector.getData(page).length, 1);
+
+    page.emit('request', request);
+
+    assert.equal(collector.getData(page).length, 2);
+  });
 });