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

Improve typescript accuracy #5168

Closed
connorjclark opened this issue Nov 15, 2019 · 4 comments
Closed

Improve typescript accuracy #5168

connorjclark opened this issue Nov 15, 2019 · 4 comments

Comments

@connorjclark
Copy link
Collaborator

connorjclark commented Nov 15, 2019

The intended way to use puppeteer + typescript today is by installing @types/puppeteer. However, there are some gaps in these types:

These discrepancies can be avoided if the type definitions are generated from the source directly.

Approach 1: Generate declaration files from JS

This seemed like the ideal approach - generate a .d.ts for every module and publish on npm. Users could use the types like this:

const Browser = require('puppeteer/lib/Browser');

interface MyInterface {
  browser: Browser;
}

Ultimately, this didn't work, I think due to bugs within TS. Here's what would need to be done.

Upgrade to TS 3.7

3.7 added support of --declaration with --allowJs. microsoft/TypeScript#7546 (comment)

tsc errors for events

This pattern trips up TS when generated the declaration file:

const EventEmitter = require('events');

class Blah extends EventEmitter {...}

The error manifests like this:

image

Using const EventEmitter = require('events').EventEmitter fixes this. Yeah, that works - it was originally the only way to import EventEmitter, and has stuck around. TS doesn't error b/c @types/node references a non-exported name for the default export, but for this named export it doesn't.

But, all modules that import a puppeteer class that extends EventEmitter still get the same error:

Declaration emit for this file requires using private name 'Page' from module '"/Users/cjamcl/src/puppeteer/lib/Page"'. An explicit type annotation may unblock declaration emit.

So, this is a blocker.

Bad generated declaration files

USKeyboardLayout.js generates this .d.ts file:

declare const _exports: {
    [x: string]: KeyDefinition;
};
export = _exports;
export type KeyDefinition = {
    keyCode?: number;
    shiftKeyCode?: number;
    key?: string;
    shiftKey?: string;
    code?: string;
    text?: string;
    shiftText?: string;
    location?: number;
};

Which isn't even valid:

image

Another blocker.

:(

At this point, I gave up.

Approach 2: Convert to TypeScript

This would certainly make great types available for puppeteer, but would be a lot of work.

Approach 3: Just fixup @types/puppeteer

At the very least, I'd like to fix the lack of good CDP types in @types/puppeteer. Puppeteer internally defines these in lib/externals.d.ts. But it uses a protocol.d.ts that is generated based on the packaged Chromium version, so it makes sense to just import that file into @types/puppeteer:

import Protocol from 'puppeteer/lib/protocol';

...

export interface CDPSession extends EventEmitter {
  /**
   * Detaches session from target. Once detached, session won't emit any events and can't be used
   * to send messages.
   */
  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<T extends keyof Protocol.CommandParameters>(method: T, parameters?: Protocol.CommandParameters[T]): Promise<Protocol.CommandReturnValues[T]>;
}

This requires publishing protocol.d.ts to npm, and tweaking the script that generates that file to export default Protocol; so it can be imported.

With these changes, the following would be well-typed:

const flattenedDocument = await session.send('DOM.getFlattenedDocument', { depth: -1 }); // errors if command name + param type do not match up

session.on('CSS.styleSheetAdded', styleSheetData => { ... }); // event data is properly typed

Some docs on how to use TypeScript would be good. Basically:

  1. install @types/puppeteer
  2. Using js and jsdoc ...
/** @typedef {import('puppeteer').Browser} Browser */
/** @typedef {import('puppeteer').CDPSession} CDPSession */
/** @typedef {import('puppeteer').Page} Page */

/**
 * @typedef MyType
 * @property {Browser} browser
 * @property {CDPSession} session
 * @property {Page} page
 */
@JoelEinbinder
Copy link
Collaborator

JoelEinbinder commented Nov 16, 2019

Take a look at utils/doclint/generate_types

@SimonSchick
Copy link

@connorjclark I amended the fix for the emulate types in my puppeteer v2 PR DefinitelyTyped/DefinitelyTyped#40284

@stale
Copy link

stale bot commented Jun 27, 2022

We're marking this issue as unconfirmed because it has not had recent activity and we weren't able to confirm it yet. It will be closed if no further activity occurs within the next 30 days.

@stale stale bot added the unconfirmed label Jun 27, 2022
@stale
Copy link

stale bot commented Jul 27, 2022

We are closing this issue. If the issue still persists in the latest version of Puppeteer, please reopen the issue and update the description. We will try our best to accomodate it!

@stale stale bot closed this as completed Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants