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.

Sign up for GitHub

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

Jump to bottom

update puppeteer to use the its generated cdp protocol types #40432

Closed
wants to merge 1 commit into from
Closed

update puppeteer to use the its generated cdp protocol types #40432

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion types/puppeteer/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

import { EventEmitter } from "events";
import { ChildProcess } from "child_process";
// @ts-ignore: suppress error if using an older version of puppeteer, which don't
Copy link
Author

Choose a reason for hiding this comment

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

The goal here is to not force users to update puppeteer - these types are for power users, so shouldn't force everyone to update. If these are missing, the CDPSession types use a sensible fallback of method: string | number | symbol and params: any, which isn't much different from the previous.

Unfortunately there doesn't seem to be a way to disable this rule. I tried:

// tslint:disable-next-line ban-ts-ignore
// tslint:disable-next-line ts-ignore

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd kindly ask you to way until my puppeteer 2.0 PR lands #40284

I'm currently working on it.

OT: This feels pretty hacky, if you really want to support TS as first class, why not move the project to TS all together?

Copy link
Author

Choose a reason for hiding this comment

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

Porting puppeteer to TS would be a huge effort.

Yeah it's pretty hacky - I think the "don't force to update for no benefit" constraint can be avoided if we 1) wait for next puppeteer version to be released (@mathiasbynens, maybe we could do a patch release?), which will have this protocol file published and 2) tack this change onto your PR, setting the minimum version to the next (unreleased 2.0.1) version

Copy link
Contributor

Choose a reason for hiding this comment

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

@connorjclark iirc the // Type definitions for PACKAGE MAJOR.MINOR line does not support specifying patch versions (which makes sense since the public API shouldn't change).
I think it would be reasonable to do a minor release in this case.

// publish types for the protocol.
import Protocol from 'puppeteer/lib/protocol';

import * as errors from "./Errors";
import * as devices from "./DeviceDescriptors";
Expand Down Expand Up @@ -2155,10 +2158,13 @@ export interface CDPSession extends EventEmitter {
*/
detach(): Promise<void>;

on<T extends keyof Protocol.Events>(event: T, listener: (arg: Protocol.Events[T]) => void): this;

/**
* @param method Protocol method name
* @param parameters Protocol parameters
*/
send(method: string, params?: object): Promise<object>;
send<T extends keyof Protocol.CommandParameters>(method: T, parameters?: Protocol.CommandParameters[T]): Promise<Protocol.CommandReturnValues[T]>;
}

export interface Coverage {
Expand Down