core(tsc): update debugger protocol type checking

[brendankenny]

Blocked on ChromeDevTools/devtools-protocol#113

The new tuple types in TypeScript 3.0 allow us to better represent protocol payloads whether an object, an optional object, or empty. See ChromeDevTools/devtools-protocol#113 for details. This allows us to get rid of a bunch of fiddly bits and simplify our StrictEventEmitter type to just a few lines.

We can also now

• defer to devtools-protocol for types instead of generating them ourselves, deleting the generator and the type definition file
• define methods like Driver.on, Connection.sendCommand, etc in the class body again instead of manually on the prototype at the end of the file, finally deleting some of the awkwardness debt left by core(tsc): add type checking to use of CRDP events #4886 and core(tsc): add type checking to remote protocol commands #4914 due to TS2.8 stuff.

One possibly controversial thing: I basically reverted the silent option from #2279 as it makes the sendCommand interface difficult to define. If we really want to, I think we can make it work (in a super ugly way), but based on the on-going #1091 it didn't seem to help the Tracing.start issue. Open to thoughts there.

[brendankenny]

✂️ ✂️ ✂️ ✂️ ✂️

[brendankenny]

only slightly annoying thing the compiler makes us do, and I think the best choice (it's difficult to go from optional object property to function argument and back to optional property in current JS and express it elegantly, regardless of type checking).

Another way might be to make StrictEventEmitter.emit() less strict (allowing undefined even if the event has no params) than the callbacks themselves since we only use it internally to Driver and Connection.

[paulirish]

this looks rad.

i'm ok with dropping the endTraceIfStarted, btw.

[paulirish]

nice!

while i do see these here https://chromedevtools.github.io/devtools-protocol/tot/Page#type-ResourceType

i don't see them here: https://github.com/ChromeDevTools/devtools-frontend/blob/master/front_end/common/ResourceType.js

but i guess that's because we're now ahead of the frontend. :)

[patrickhulce]

but i guess that's because we're now ahead of the frontend. :)

🎉 💣 💥 💯 💃 🕺 🥇

music to my ears!!!

[denar90]

Can we expect to have this merged before next release?

[brendankenny]

Yes, when ChromeDevTools/devtools-protocol#113 ships to npm I just need to update and we should be able to land it

[denar90]

Thanks for the info :)

[brendankenny]

no longer a WIP :)

[brendankenny]

this is unfortunate to ignore, but will have to wait on the ts team for the fix.

Basically _eventEmitter.emit() correctly checks that for every protocol event method name (e.g. 'Profiler.consoleProfileStarted'), it will have a matching event params (e.g. LH.Crdp.Profiler.ConsoleProfileStartedEvent).

The event object passed into this function is a giant union of possible method/pair objects like:

{method: 'Profiler.consoleProfileStarted', params: LH.Crdp.Profiler.ConsoleProfileStartedEvent} |
{method: 'Runtime.bindingCalled', params: LH.Crdp.Runtime.BindingCalledEvent} |
{method: 'CSS.fontsUpdated', params: LH.Crdp.CSS.FontsUpdatedEvent} |
...

It's a discriminated union, so in theory the compiler could check each union member separately, which would pass the _eventEmitter.emit() check.

Currently, however, the compiler doesn't narrow the type based on method, so instead of taking each union member separately, it takes the union of all possible property values. The second argument passed to _eventEmitter.emit() ends up being LH.Crdp.Profiler.ConsoleProfileStartedEvent | LH.Crdp.Runtime.BindingCalledEvent | LH.Crdp.CSS.FontsUpdatedEvent | ....

When _eventEmitter.emit() starts checking that for every method ('Profiler.consoleProfileStarted') there is the correct params (LH.Crdp.Profiler.ConsoleProfileStartedEvent), it sees that full union instead and so rightly shows an error since only one member of the union would have been the correct type.

As noted, this will hopefully be fixed by microsoft/TypeScript#22348, currently aimed at the typescript 3.2ish timeframe.

[brendankenny]

Also, it's slightly confusing, but the similar checks in the surrounding methods (e.g. other uses of eventEmitter) are correctly type checked. The key is that here event is an object with method and params as properties. If method and params had come in as separate function parameters (as they do elsewhere) or as a tuple, the compiler would be able to correctly type them.

[paulirish]

LETS DO THIS

[denar90]

thanks 💃

[denar90]

@brendankenny @paulirish one more thing
do you have any info about next (maybe small/minor/teeny-tiny) release? :)

[denar90]

Not relevant question, thanks it's already released)