core(tsc): add type checking to use of CRDP events #4886

brendankenny · 2018-03-28T21:38:37Z

OK! Took a bit longer than I hoped, but working now and hopefully a maintainable solution because it adds significant usability improvements.

Adds type checking and type inference to Chrome Remote Debugging Protocol events listened to via Driver:

The emitter type is based on https://github.com/bterlson/strict-event-emitter-types/, but it turns out we can't use it as-is in JS because typescript doesn't check @implements yet, you can't do method overloading in JS, and overloading in a typescript d.ts file isn't compatible with union types (which we can use in JS).

A breakdown of this PR:

third-party/strict-event-emitter-types, LICENSE and index.d.ts.
This is a forked version of strict-event-emitter-types replacing method overloads with type unions. The changes reduce the flexibility of the emit interface, so unfortunately I don't think we'll be able to upstream these changes (at least until future TS changes), but the loss of flexibility turns out to have basically no effect on us since our events are coming over the wire anyways.
scripts/extract-crdp-mapping.js and typings/crdp-mapping.d.ts:
extract-crdp-mapping.js walks the protocol d.ts file using the typescript compiler API and pulls out an interface with keys that are all the event names (Console.messageAdded, Debugger.breakpointResolved, etc) mapped to the expected event payload type (LH.Crdp.Console.MessageAddedEvent, LH.Crdp.Debugger.BreakpointResolvedEvent, etc).

extract-crdp-mapping.js can be run with yarn update:crdp-typings and is saved to typings/crdp-mapping.d.ts so it can be used for typing. The event map is used to tell StrictEventEmitter what event payload type is expected to be received (or emitted) based on the event string.

When protocol-dts-generator for devtools-protocol typescript types ChromeDevTools/devtools-protocol#90 lands we can hopefully get rid of both of these files and upstream the interface into the official type declaration as Crdp.AllEvents or whatever.
typings/protocol.d.ts:
Creates some union types out of the event map in crdp-mapping.d.ts that are compatible with a StrictEventEmitter that's created from the same event map.
all the rest (connection and driver, mostly)
Altered to use these libraries. The main pain point is that typescript doesn't support constrained templates in JSDocs right now, so there's no way to express the constraints we want to on Driver.on, Connection.on, etc using the normal @param @return jsdocs. However the body of @type does support most of what we want to do, and while you can't add @type to a method definition in a Class body, you can add them to a function expression assigned to the class prototype outside that body :)

So the on/once/off methods have been pulled out (unaltered) and defined at the bottom of the files, and then we can conveniently just type them as the event emitter method (e.g. CrdpEventEmitter['on']) and it's not too messy. Any other changes are just to pipe those specific types through.

The PR is a bit long, but as noted, hopefully extract-crdp-mapping.js and crdp-mapping.d.ts can be deleted in the near future.

wardpeet · 2018-03-28T21:41:28Z

#excited

brendankenny · 2018-03-28T21:43:13Z

lighthouse-core/gather/connections/connection.js

    if (!this._eventEmitter) {
      throw new Error('Attempted to emit event after connection disposed.');
    }
-    this._eventEmitter.emit('notification', {method, params});
+
+    this._eventEmitter.emit('protocolevent', eventMessage);


discussed this with @paulirish earlier. Since this event emitter can only emit protocol events (and Driver is the only receiver), it makes sense to make the single event it handles be something more descriptive than "notification" :)

brendankenny · 2018-03-28T21:44:47Z

lighthouse-core/gather/driver.js

@@ -946,8 +911,8 @@ class Driver {
  endTrace() {
    return new Promise((resolve, reject) => {
      // When the tracing has ended this will fire with a stream handle.
-      this.once('Tracing.tracingComplete', streamHandle => {
-        this._readTraceFromStream(streamHandle)
+      this.once('Tracing.tracingComplete', completeEvent => {


note that these are now type checked (not any!) but they are also inferred correctly from the event string, so no annotation needed :)

brendankenny · 2018-03-28T21:48:35Z

package.json

@@ -83,7 +84,7 @@
    "postinstall-prepare": "^1.0.1",
    "puppeteer": "^1.1.1",
    "sinon": "^2.3.5",
-    "typescript": "^2.8.0-rc",
+    "typescript": "2.9.0-dev.20180323",


typescript 2.8.1 just came out with all the typescript features we need, but unfortunately the PR that extends them to javascript didn't get pulled over to the 2.8 branch and now won't be seen until 2.9 (microsoft/TypeScript#22692 (comment)). I did some testing and this is the closest nightly to the 2.8.1 release that has the commit we need and is stable, so I went with this. They're on a 6-8 week cadence, so hopefully we can switch to 2.9 in May-ish

patrickhulce

I mostly glazed over trying to read bterlson's d.ts again 😆 but js changes seem pretty minimal, awesome job! 💯

patrickhulce · 2018-03-28T22:09:12Z

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

+const path = require('path');
+const ts = require('typescript');
+
+const crdpTypingFile = './node_modules/vscode-chrome-debug-core/lib/crdp/crdp.d.ts';


shall we require resolve this one?

patrickhulce · 2018-03-28T22:12:50Z

typings/externs.d.ts

    export import Crdp = _Crdp;
+    export type StrictEventEmitter<TEmitterType, TEventRecord, TEmitRecord = TEventRecord> =


I never really got why we need to keep the EventEmitter component, while we're aliasing shall we just flip and default it to EventEmitter? :)

ooh, good idea

patrickhulce · 2018-03-28T22:14:43Z

typings/protocol.d.ts

+  [K in keyof LH.CrdpEvents]: {
+    method: K,
+    // Drop void for `undefined` (so a JS value is valid). See above TODO
+    params: LH.CrdpEvents[K] extends void ? undefined: LH.CrdpEvents[K]


so here's the magic line, huh. incredible 💘 that tsc

…ation

patrickhulce

LGTM let's kick the 🚗 on this 💯

brendankenny · 2018-03-30T20:27:21Z

@paulirish also gave the 👍

brendankenny requested review from patrickhulce and paulirish as code owners March 28, 2018 21:38

devtools-bot added the waiting4reviewer label Mar 28, 2018

brendankenny changed the title ~~core(tsc): add type checking to use of CRDP events~~ core(tsc): add type checking to use of CRDP events Mar 28, 2018

brendankenny commented Mar 28, 2018

View reviewed changes

patrickhulce reviewed Mar 28, 2018

View reviewed changes

brendankenny added 3 commits March 28, 2018 17:26

core(tsc): add third-party strict-event-emitter-types lib for modific…

1437740

…ation

core(tsc): add type checking to uses of CRDP events

7608fb7

feedback

271e08a

brendankenny force-pushed the tsevents branch from f17992f to 271e08a Compare March 29, 2018 00:27

patrickhulce approved these changes Mar 30, 2018

View reviewed changes

brendankenny merged commit e26b2cf into master Mar 30, 2018

brendankenny deleted the tsevents branch March 30, 2018 20:27

This was referenced Apr 3, 2018

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

Merged

Add strict type checking to protocol interactions #4870

Closed

paulirish removed the waiting4reviewer label Apr 30, 2018

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 use of CRDP events #4886

core(tsc): add type checking to use of CRDP events #4886

brendankenny commented Mar 28, 2018 •

edited

Loading

wardpeet commented Mar 28, 2018

brendankenny Mar 28, 2018

brendankenny Mar 28, 2018

brendankenny Mar 28, 2018

patrickhulce left a comment •

edited

Loading

patrickhulce Mar 28, 2018

patrickhulce Mar 28, 2018

brendankenny Mar 29, 2018

patrickhulce Mar 28, 2018

patrickhulce left a comment

brendankenny commented Mar 30, 2018

		export import Crdp = _Crdp;
		export type StrictEventEmitter<TEmitterType, TEventRecord, TEmitRecord = TEventRecord> =

core(tsc): add type checking to use of CRDP events #4886

core(tsc): add type checking to use of CRDP events #4886

Conversation

brendankenny commented Mar 28, 2018 • edited Loading

wardpeet commented Mar 28, 2018

brendankenny Mar 28, 2018

Choose a reason for hiding this comment

brendankenny Mar 28, 2018

Choose a reason for hiding this comment

brendankenny Mar 28, 2018

Choose a reason for hiding this comment

patrickhulce left a comment • edited Loading

Choose a reason for hiding this comment

patrickhulce Mar 28, 2018

Choose a reason for hiding this comment

patrickhulce Mar 28, 2018

Choose a reason for hiding this comment

brendankenny Mar 29, 2018

Choose a reason for hiding this comment

patrickhulce Mar 28, 2018

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny commented Mar 30, 2018

brendankenny commented Mar 28, 2018 •

edited

Loading

patrickhulce left a comment •

edited

Loading