core(tsc): add type checking to remote protocol commands #4914

brendankenny · 2018-04-03T05:28:03Z

This PR adds type checking/inference for Driver.sendCommand (to go along with typing events in #4886).

Like #4886, this PR has changes to scripts/extract-crdp-mapping.js to extract all the protocol commands which are then written to typings/crdp-mapping.d.ts. Again, hopefully most of this can be upstreamed into devtools-protocol. This time the types need both the options params object (if any) associated with the command but also the command's return type (if any).

The other set of changes are in Driver, Connection, and ExtensionConnection (everything else is just minor fixes for any type errors that popped up after all the above changes). This time sendCommand is pulled out of the class bodies for stricter typing. Getting the typing correct is a little harder than it was for the events because this time we aren't tying together an event name and payload type, but two function parameters and a return type.

Typescript types (in typescript or jsdoc) can express that one property of an object is defined only for certain values of a different property (or similarly, for an event payload in a callback and an event name), but typescript can't yet express that a function parameter is not to be provided depending on the value of another parameter (there's a proposal to enable this, which is now on the typescript roadmap, but under the "Future" heading with no planned implementation schedule).

As a result, the solution is a little more awkward, but not too bad. The function is defined with somewhat loose argument types, but when added to the class prototype it's cast to a much stricter type as the external interface. This allows casting with an overloaded type signature, which can create a one argument version (for commands like DOM.enable) or a two argument version so options can be passed (for e.g. DOM.querySelector), depending on the first argument (the command string).

One last wrinkle: for commands that have all optional options that we don't use, we have often just dropped the second argument when calling (e.g. for Network.enable). I've added void as one option for the params object for those commands, so they actually show up in both the one and two argument versions of sendCommand

brendankenny · 2018-04-03T05:36:09Z

lighthouse-core/gather/driver.js

@@ -656,11 +637,9 @@ class Driver {
      return this._isolatedExecutionContextId;
    }

-    /** @type {LH.Crdp.Page.GetResourceTreeResponse} */


typed responses lets us get rid of a lot of these explicit types :)

brendankenny · 2018-04-03T05:39:23Z

lighthouse-core/lib/emulation.js

@@ -86,7 +86,7 @@ function enableNexus5X(driver) {
    // Network.enable must be called for UA overriding to work
    driver.sendCommand('Network.enable'),
    driver.sendCommand('Network.setUserAgentOverride', NEXUS5X_USERAGENT),
-    driver.sendCommand('Emulation.setTouchEmulationEnabled', {
+    driver.sendCommand('Page.setTouchEmulationEnabled', {


first real bug caught with type checks? Haven't confirmed that it was a problem, but there is both Emulation.setTouchEmulationEnabled and Page.setTouchEmulationEnabled, but only the Page one takes these params

ha! yeah I think this might have been a small bug that normally didn't matter.
I think we want Emulation.setEmitTouchEventsForMouse instead.

that's what devtools uses when you toggle device type:

seems like you're fixing this in a separate PR?

can you write up the list of changes that are on their way?

sorry, I can fix this :)

patrickhulce

coool beans 😎

patrickhulce · 2018-04-03T16:21:13Z

typings/protocol.d.ts

+     * params object.
+     */
+    export type SendCommand = {
+      <C extends VoidParamsKeys>(method: C, params?: void, cmdOpts?: {silent?: boolean}): Promise<CrdpCommands[C]['returnType']>;


a little unfortunate that types make us do an extends and we can't do in

patrickhulce · 2018-04-03T16:23:07Z

lighthouse-core/gather/driver.js

@@ -809,11 +785,9 @@ class Driver {
   * @return {Promise<Array<Element>>} The found elements, or [], resolved in a promise
   */
  async querySelectorAll(selector) {
-    /** @type {LH.Crdp.DOM.GetDocumentResponse} */
-    const documentResponse = await this.sendCommand('DOM.getDocument');
+    const documentResponse = await this.sendCommand('DOM.getDocument', {});


I thought the | void would let us omit the parameters if they're all optional?

whoops, yes, those were from an earlier version of the PR

patrickhulce · 2018-04-03T16:26:32Z

lighthouse-core/gather/connections/connection.js

@@ -111,7 +98,7 @@ class Connection {
      // just log these occurrences.
      const error = object.error && object.error.message;
      log.formatProtocol(`disowned method <= browser ${error ? 'ERR' : 'OK'}`,
-          {method: object.method, params: error || object.result}, 'verbose');
+          {method: 'UNKNOWN', params: error || object.result}, 'verbose');


do we not get back a method from disowned messages? seems like || 'UNKOWN' would work if we do :)

do we not get back a method from disowned messages?

we can check on the Chrome side if it's possible, but I did a bunch of runs watching the properties of all messages sent back over the protocol and never got a method on a non-event response

paulirish

the .d.ts is now in the devtools-protocol package: https://cdn.jsdelivr.net/npm/devtools-protocol@0.0.547074/types/protocol.d.ts

want to switch to that and off of vscode-chrome-debug-core?

paulirish · 2018-04-03T18:31:24Z

lighthouse-core/lib/emulation.js

@@ -86,7 +86,7 @@ function enableNexus5X(driver) {
    // Network.enable must be called for UA overriding to work
    driver.sendCommand('Network.enable'),
    driver.sendCommand('Network.setUserAgentOverride', NEXUS5X_USERAGENT),
-    driver.sendCommand('Emulation.setTouchEmulationEnabled', {
+    driver.sendCommand('Page.setTouchEmulationEnabled', {


ha! yeah I think this might have been a small bug that normally didn't matter.
I think we want Emulation.setEmitTouchEventsForMouse instead.

that's what devtools uses when you toggle device type:

paulirish · 2018-04-03T18:33:59Z

lighthouse-core/scripts/extract-crdp-mapping.js

+ * @param {string} domainName
+ * @return {Map<string, {paramsType: string, returnType: string}>}
+ */
+function getCommandMap(sourceRoot, domainName) {


do you want to upstream all this into devtools-protocol?

do you want to upstream all this into devtools-protocol?

the event map (just 'Domain.eventName': Crdp.EventPayloadType) I think could easily live upstream, it's so simple. The command map seems more specialized to our use case, so I'm not sure. We could propose it to everyone involved in ChromeDevTools/devtools-protocol#90 and see what they think

cc @@JoelEinbinder whatchu think homeboy

he sounded 👍 for upstreaming (if not for using in pptr)

brendankenny · 2018-04-03T18:39:12Z

the .d.ts is now in the devtools-protocol package. want to switch to that and off of vscode-chrome-debug-core?

yeah, it was just going to require changes to this and the previous event stuff because they changed the structure a bit. I was going to do that in a quick follow up PR.

brendankenny · 2018-04-03T22:28:27Z

yah?

paulirish · 2018-04-03T22:33:01Z

lighthouse-core/lib/emulation.js

@@ -86,7 +86,7 @@ function enableNexus5X(driver) {
    // Network.enable must be called for UA overriding to work
    driver.sendCommand('Network.enable'),
    driver.sendCommand('Network.setUserAgentOverride', NEXUS5X_USERAGENT),
-    driver.sendCommand('Emulation.setTouchEmulationEnabled', {
+    driver.sendCommand('Page.setTouchEmulationEnabled', {


seems like you're fixing this in a separate PR?

can you write up the list of changes that are on their way?

brendankenny · 2018-04-03T22:48:54Z

can you write up the list of changes that are on their way?

I believe the only (direct) next steps are to

upstream CrdpEvents and CrdpCommands to devtools-protocol
delete our local scripts/extract-crdp-mappings.js and typings/crdp-mappings.d.ts and remove vscode-chrome-debug-core as a dep
switch over to devtools-protocol for typing the protocol

brendankenny · 2018-04-04T00:08:56Z

@patrickhulce also gave the 👍

brendankenny requested review from patrickhulce and paulirish as code owners April 3, 2018 05:28

devtools-bot added the waiting4reviewer label Apr 3, 2018

core(tsc): add type checking to remote protocol commands

8c203db

brendankenny force-pushed the tscommands branch from d3f988c to 8c203db Compare April 3, 2018 05:33

brendankenny commented Apr 3, 2018

View reviewed changes

patrickhulce reviewed Apr 3, 2018

View reviewed changes

remove explicit {}

0d3b8a6

paulirish reviewed Apr 3, 2018

View reviewed changes

paulirish approved these changes Apr 3, 2018

View reviewed changes

Emulation.setEmitTouchEventsForMouse

dc55a4b

brendankenny merged commit 30443e6 into master Apr 4, 2018

brendankenny deleted the tscommands branch April 4, 2018 00:09

brendankenny mentioned this pull request Apr 4, 2018

Add strict type checking to protocol interactions #4870

Closed

paulirish removed the waiting4reviewer label Apr 30, 2018

FiloSottile mentioned this pull request Jun 17, 2018

Add type checking to CDPSession.send puppeteer/puppeteer#2758

Closed

patrickhulce mentioned this pull request Jul 12, 2018

When running Lighthouse from Webpagetest there is no support of touch device #5659

Closed

brendankenny mentioned this pull request Aug 15, 2018

core(tsc): update debugger protocol type checking #5836

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tsc): add type checking to remote protocol commands #4914

core(tsc): add type checking to remote protocol commands #4914

brendankenny commented Apr 3, 2018

brendankenny Apr 3, 2018

brendankenny Apr 3, 2018

paulirish Apr 3, 2018

paulirish Apr 3, 2018

brendankenny Apr 3, 2018

patrickhulce left a comment

patrickhulce Apr 3, 2018

patrickhulce Apr 3, 2018

brendankenny Apr 3, 2018

patrickhulce Apr 3, 2018

brendankenny Apr 3, 2018

paulirish left a comment

paulirish Apr 3, 2018

paulirish Apr 3, 2018

brendankenny Apr 3, 2018 •

edited

Loading

paulirish Apr 3, 2018

brendankenny Apr 3, 2018

brendankenny commented Apr 3, 2018

brendankenny commented Apr 3, 2018

paulirish Apr 3, 2018

brendankenny commented Apr 3, 2018

brendankenny commented Apr 4, 2018

core(tsc): add type checking to remote protocol commands #4914

core(tsc): add type checking to remote protocol commands #4914

Conversation

brendankenny commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 3, 2018

brendankenny commented Apr 3, 2018

Choose a reason for hiding this comment

brendankenny commented Apr 3, 2018

brendankenny commented Apr 4, 2018

brendankenny Apr 3, 2018 •

edited

Loading