From 866d34ee1122e89eab00743246676845bb065968 Mon Sep 17 00:00:00 2001
From: Jack Franklin <jacktfranklin@chromium.org>
Date: Thu, 25 Mar 2021 11:26:35 +0000
Subject: [PATCH] fix: type page event listeners correctly (#6891)

This PR fixes the fact that currently if you have:

```ts
page.on('request', request => {

})
```

Then `request` will be typed as `any`. We can fix this by defining an
interface of event name => callback argument type, and looking that up
when you call `page.on`.

Also includes a drive-by fix to ensure we convert response headers to
strings, and updates the types accordingly.
---
 src/common/HTTPRequest.ts                     | 11 ++--
 src/common/Page.ts                            | 50 +++++++++++++++++++
 .../ts-cjs-import-cjs-output/good.ts          |  3 ++
 utils/doclint/check_public_api/index.js       | 21 ++++++++
 4 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/src/common/HTTPRequest.ts b/src/common/HTTPRequest.ts
index 5adb0cd578c..7b581ee936f 100644
--- a/src/common/HTTPRequest.ts
+++ b/src/common/HTTPRequest.ts
@@ -40,7 +40,10 @@ export interface ContinueRequestOverrides {
  */
 export interface ResponseForRequest {
   status: number;
-  headers: Record<string, string>;
+  /**
+   * Optional response headers. All values are converted to strings.
+   */
+  headers: Record<string, unknown>;
   contentType: string;
   body: string | Buffer;
 }
@@ -346,7 +349,7 @@ export class HTTPRequest {
    *
    * @param response - the response to fulfill the request with.
    */
-  async respond(response: ResponseForRequest): Promise<void> {
+  async respond(response: Partial<ResponseForRequest>): Promise<void> {
     // Mocking responses for dataURL requests is not currently supported.
     if (this._url.startsWith('data:')) return;
     assert(this._allowInterception, 'Request Interception is not enabled!');
@@ -361,7 +364,9 @@ export class HTTPRequest {
     const responseHeaders: Record<string, string> = {};
     if (response.headers) {
       for (const header of Object.keys(response.headers))
-        responseHeaders[header.toLowerCase()] = response.headers[header];
+        responseHeaders[header.toLowerCase()] = String(
+          response.headers[header]
+        );
     }
     if (response.contentType)
       responseHeaders['content-type'] = response.contentType;
diff --git a/src/common/Page.ts b/src/common/Page.ts
index aff7615a39f..db30be48b74 100644
--- a/src/common/Page.ts
+++ b/src/common/Page.ts
@@ -329,6 +329,35 @@ export const enum PageEmittedEvents {
   WorkerDestroyed = 'workerdestroyed',
 }
 
+/**
+ * Denotes the objects received by callback functions for page events.
+ *
+ * See {@link PageEmittedEvents} for more detail on the events and when they are
+ * emitted.
+ * @public
+ */
+export interface PageEventObject {
+  close: never;
+  console: ConsoleMessage;
+  dialog: Dialog;
+  domcontentloaded: never;
+  error: Error;
+  frameattached: Frame;
+  framedetached: Frame;
+  framenavigated: Frame;
+  load: never;
+  metrics: { title: string; metrics: Metrics };
+  pageerror: Error;
+  popup: Page;
+  request: HTTPRequest;
+  response: HTTPResponse;
+  requestfailed: HTTPRequest;
+  requestfinished: HTTPRequest;
+  requestservedfromcache: HTTPRequest;
+  workercreated: WebWorker;
+  workerdestroyed: WebWorker;
+}
+
 class ScreenshotTaskQueue {
   _chain: Promise<Buffer | string | void>;
 
@@ -559,6 +588,27 @@ export class Page extends EventEmitter {
     return this._javascriptEnabled;
   }
 
+  /**
+   * Listen to page events.
+   */
+  public on<K extends keyof PageEventObject>(
+    eventName: K,
+    handler: (event: PageEventObject[K]) => void
+  ): EventEmitter {
+    // Note: this method only exists to define the types; we delegate the impl
+    // to EventEmitter.
+    return super.on(eventName, handler);
+  }
+
+  public once<K extends keyof PageEventObject>(
+    eventName: K,
+    handler: (event: PageEventObject[K]) => void
+  ): EventEmitter {
+    // Note: this method only exists to define the types; we delegate the impl
+    // to EventEmitter.
+    return super.once(eventName, handler);
+  }
+
   /**
    * @param options - Optional waiting parameters
    * @returns Resolves after a page requests a file picker.
diff --git a/test-ts-types/ts-cjs-import-cjs-output/good.ts b/test-ts-types/ts-cjs-import-cjs-output/good.ts
index d055bf6f6a5..a17870ed0a3 100644
--- a/test-ts-types/ts-cjs-import-cjs-output/good.ts
+++ b/test-ts-types/ts-cjs-import-cjs-output/good.ts
@@ -5,6 +5,9 @@ async function run() {
   const devices = puppeteer.devices;
   console.log(devices);
   const page = await browser.newPage();
+  page.on('request', (request) => {
+    const resourceType = request.resourceType();
+  });
   const div = (await page.$('div')) as puppeteer.ElementHandle<
     HTMLAnchorElement
   >;
diff --git a/utils/doclint/check_public_api/index.js b/utils/doclint/check_public_api/index.js
index bc0bf9ec3e7..e524e52b6a7 100644
--- a/utils/doclint/check_public_api/index.js
+++ b/utils/doclint/check_public_api/index.js
@@ -864,6 +864,27 @@ function compareDocumentations(actual, expected) {
           expectedName: 'Array<Permission>',
         },
       ],
+      [
+        'Method HTTPRequest.respond() response.body',
+        {
+          actualName: 'string|Buffer',
+          expectedName: 'Object',
+        },
+      ],
+      [
+        'Method HTTPRequest.respond() response.contentType',
+        {
+          actualName: 'string',
+          expectedName: 'Object',
+        },
+      ],
+      [
+        'Method HTTPRequest.respond() response.status',
+        {
+          actualName: 'number',
+          expectedName: 'Object',
+        },
+      ],
     ]);
 
     const expectedForSource = expectedNamingMismatches.get(source);