Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: type page event listeners correctly #6891

Merged
merged 1 commit into from Mar 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/common/HTTPRequest.ts
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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!');
Expand All @@ -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;
Expand Down
50 changes: 50 additions & 0 deletions src/common/Page.ts
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding a [event: string]: any at the bottom here to make sure we don't break places that use other event names? Or is that intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather it fail explicitly because we're missing a type.

}

class ScreenshotTaskQueue {
_chain: Promise<Buffer | string | void>;

Expand Down Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions test-ts-types/ts-cjs-import-cjs-output/good.ts
Expand Up @@ -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
>;
Expand Down
21 changes: 21 additions & 0 deletions utils/doclint/check_public_api/index.js
Expand Up @@ -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);
Expand Down