update protocol-dts-generator to add command and event to types mapping #113

brendankenny · 2018-08-13T17:34:30Z

Previously the protocol's typescript definition included not just the types used by the protocol but also a set of generated methods that a theoretical interface for the API could use, e.g. the protocol command Console.clearMessages has a matching interface entry Protocol.ConsoleApi.clearMessages(): Promise<void>;.

That can be confusing because all the different clients have exposed the protocol in different ways. e.g. lighthouse supports await sendCommand('Console.clearMessages'), chrome-remote-interface has await client.Console.clearMessages(), etc.

This PR removes those methods (leaving all the type definitions), and instead builds foundational types that map events to their payloads and commands to their parameters and responses. e.g.

export module ProtocolMapping {
  export interface Events {
    'Console.messageAdded': Protocol.Console.MessageAddedEvent;
    'Debugger.breakpointResolved': Protocol.Debugger.BreakpointResolvedEvent;
    // ...
  }

  export interface Commands {
    'Console.clearMessages': {
      paramsType: [];
      returnType: void;
    };
    'Debugger.continueToLocation': {
      paramsType: [Protocol.Debugger.ContinueToLocationRequest];
      returnType: void;
    };
  }
}

Libraries that use the protocol can then uses these mappings to build their own type definitions (in their own repos or in DefinitelyTyped)

This is similar to the model Lighthouse and Puppeteer currently use. In Lighthouse we take this mapping and then transform it for different purposes (for instance, this defines the union of possible JSON responses from sending protocol commands).

Also cleans up the dts generator a bit.

brendankenny · 2018-08-13T17:43:45Z

One new thing is that this uses the new tuple features in TypeScript 3.0, so I've kept the mappings in a new file in case users of older versions of typescript need the base object types.

The new tuple syntax allow bypassing a lot of the backflips we had to do because typescript struggles at the boundary of function overloading and type unions. This was an issue because we're building the types up dynamically but some commands have parameters and some don't (and some have parameters that are entirely optional). .

Now ParamType, (void | ParamType), and void are unified as [ParamType], [ParamType?], and [], all of which can now just be spread in a single function declaration instead of having to deal with overloading at all.

paulirish · 2018-08-13T17:45:20Z

cc @nojvek @JoelEinbinder wanna take a look?

brendankenny · 2018-08-13T18:19:11Z

types/protocol.d.ts

@@ -2,94 +2,14 @@
 * Auto-generated by protocol-dts-generator.ts, do not edit manually. *
 **********************************************************************/

-export namespace Protocol {
+export module Protocol {


the diff gets a little wonky depending on how far you scroll but the only changes in here are

~~s/namespace/module~~

DomainApi interfaces deleted

whitespace adjustments

descriptions for event types now included in jsdoc

why module but not namespaces?

Initially added the DomainApi so the types work with noice-rpc-json out of the box. I believe vscode-chrome-debugger also uses this info so this could be a breaking change for some clients.

@roblourens please confirm.

looks like vscode-chrome-debug-core does indeed depend on these. We'll figure out what we should do

why module but not namespaces?

yeah, looks like you're right

paulirish · 2018-08-13T18:33:15Z

scripts/protocol-dts-generator.ts

-    const eventDefs = domain.events ? domain.events.map(e => emitEvent(e, domainName)) : []
+    emitOpenBlock(`export module ${domainName}`)
+    if (domain.types) domain.types.forEach(emitDomainType)
+    if (domain.commands) domain.commands.forEach(c => emitCommand(c))


forEach(emitCommand) right?

ah yeah, good call

paulirish · 2018-08-13T18:33:28Z

scripts/protocol-dts-generator.ts

+    emitOpenBlock(`export module ${domainName}`)
+    if (domain.types) domain.types.forEach(emitDomainType)
+    if (domain.commands) domain.commands.forEach(c => emitCommand(c))
+    if (domain.events) domain.events.forEach(e => emitEvent(e))


forEach(emitEvent)

paulirish · 2018-08-13T18:34:56Z

scripts/protocol-dts-generator.ts

        return `${getPropertyType(prop.items)}[]`
-    else if (prop.type == 'object')
-        return `any`
-    else if (prop.type == 'function')


just checking: function and lambda dont need to be handled any longer?

just checking: function and lambda dont need to be handled any longer?

right. They were types constructed just by the dts generator itself (in the old versions of emitCommand and emitEvent) to be able to use the same emitInterface pipeline to output the generated command and event listener functions.

(similar to the new getCommandMapping and getEventMapping, but they just reuse the RefType that the protocol already uses for cross references within itself)

brendankenny · 2018-08-14T00:08:34Z

Update to use these definitions in Lighthouse: GoogleChrome/lighthouse@049af19...7488a41
(if this PR lands, will also delete crdp-mapping.d.ts in favor of the protocol-mapping.d.ts from this PR).

sendCommand in driver.js shows the much simpler rest/spread of Protocol.Commands[C]['paramsType'], which automatically handles the 0, 1, and optional argument cases (rather than dealing with overloading sendCommand to accommodate void and non-void parameters)

paulirish

lgtm

anyone else want to look before we merge?

paulirish · 2018-08-15T01:13:53Z

scripts/protocol-dts-generator.ts

-    }
+const getPropertyDef = (prop: P.PropertyType): string => {
+    // Quote key if it has a . in it.
+    const propName = /\./.test(prop.name) ? `'${prop.name}'` : prop.name


i'm a sucker for str.includes but w/e

I'd agree that str.includes would be more readable

I'd agree that str.includes would be more readable

lol ok fine

JoelEinbinder · 2018-08-15T01:19:00Z

Concept looks sound.

nojvek · 2018-08-15T04:45:48Z

I’d like to take a pass. As a new dad I don’t get as much time for OS tings. I’ll do it in the next couple of hours though. On an unrelated note: I’m quite envious of Google parental leave.

nojvek · 2018-08-15T04:52:47Z

scripts/package.json

-    "ts-node": "^5.0.1",
-    "typescript": "^2.7.2"
+    "ts-node": "^7.0.0",
+    "typescript": "3.0.1"


nojvek · 2018-08-15T04:57:04Z

scripts/protocol-dts-generator.ts

+        type: 'object',
+        name: `${domainName}.${command.name}`,
+        // TODO(bckenny): does having a description help anywhere?
+        // description: command.description,


what's the harm in having description?

what's the harm in having description?

no harm except file size and maybe ts parse time? The main thing is I couldn't find any place where the descriptions end up surfaced anyways since the mapping objects can't really be used directly, so it didn't seem worth it. I left them commented out so if it turns out we do want them you don't have to wade into the code again.

It seemed maybe better to take the command descriptions and somehow modify them to apply them to the CommandRequest and CommandResponse types, as those do show up in completions and intellisense, but I didn't see an easy way to modify the strings to apply the types. "The request parameters for a command that..." or something :)

nojvek · 2018-08-15T05:00:04Z

scripts/protocol-dts-generator.ts

-}
-
-type ParamOrFunction = P.ParameterType | P.FunctionType
+const jsProtocol: IProtocol = require('../json/js_protocol.json')


Thank you 🙏 . If latest tsc does indeed validate the require-ed json against the IProtocol type, you can remove the TODO on line 5.

If latest tsc does indeed validate the require-ed json against the IProtocol type, you can remove the TODO on line 5.

sadly it does not, the require() comes in as any, but if I do "esModuleInterop": true in tsconfig and go back to import jsProtocol from '../json/js_protocol.json'; I can't apply an interface to the import statement (and if you apply the interface later, things like property type fields have been widened to string instead of their literal values so it fails).

Weirdly the js version of this works fine

/** @typedef {import('./protocol-schema').IProtocol} IProtocol */ /** @type {IProtocol} */ const jsProtocol = require('../json/js_protocol.json')

and any changes to the schema or json are flagged as type errors.

So, leaving the TODO for now, but I can unassign you if you'd like :)

works for me as is. Don't remove the TODO just yet.

I do quite like the work tsc team having doing regarding jsdoc type checking and //@ts-check. I know Google's lingua franca is Js. So if we want this file to be JS with jsdoc type annotations, I don't mind it at all.

Doesn't have to be this PR right away.

nojvek · 2018-08-15T05:02:26Z

scripts/protocol-dts-generator.ts

    emitLine()
 }

-const emitModule = (moduleName:string, domains: P.Domain[]) => {
+const emitModule = (moduleName: string, domains: P.Domain[]) => {


ha! spaces! We should just get this tslinted so you don't have to curse me everytime you see a missing space.

We should just get this tslinted

agreed :)

nojvek · 2018-08-15T05:13:26Z

scripts/protocol-dts-generator.ts

+            // TODO: actually 'any'? or can use generic '[key: string]: string'?
+            return `any`
+        } else {
+            // hack: access indent, \n directly


You could make this into a less hacky version by implementing a startSubEmit() and endSubEmit(). startSubEmit saves old str and indent in a stack. all emitX calls go to a new str, so you can you use emitLine(), emiOpenBlock and emitCloseBlock without guilt. endSubEmit() would return the str that was generated by the emitCalls.

I know you're only using it here, but I imagine the next dev may follow hack pattern to make more hacks. Up-to-you though, don't wanna bikeshed too much.

You could make this into a less hacky version by implementing a startSubEmit() and endSubEmit()

In the original form of this PR I reworked the whole emit system just for this one spot, but it ended up seeming like way too many changes for what's just a simple reach into the indent state, so I reverted all that and did this instead.

I know you're only using it here, but I imagine the next dev may follow hack pattern to make more hacks.

Agreed, but maybe the (eventual) need for a second hack will be a better justification for the refactor, and give better insight on what form it should take.

nojvek · 2018-08-15T05:46:39Z

types/protocol.d.ts

            /**
             * Line number in the resource that generated this message (1-based).
             */
            line?: integer;
-


Don't the spaces make the file more readable?

Don't the spaces make the file more readable?

It was way too much whitespace in the mappings file and removing it from interfaces in the protocol file seemed still readable since most have a description comment block. And I didn't want to pipe in a boolean for extra lines or not :)

Can re-add spacing if others agree it's better with it.

no strong feelings but i'm fine without the spaces. it's dense but the comments already give things plenty of room.

nojvek

W4m, minor nits and ask that we don't break any existing clients.

brendankenny · 2018-08-16T23:24:29Z

ok, I added back the ProtocolApi interfaces and methods since at least vscode-chrome-debug-core uses them.

The compromise is that they're now in a different file than protocol.d.ts, so they'll have to be imported from protocol-api.d.ts. What do folks think?

Also happy to bikeshed on naming. It might be a bit weird to call it protocol-api.d.ts since it's really an API for the protocol, not the API :)

nojvek · 2018-08-17T01:00:57Z

I propose we call it protocol-rpc-api or protocol-proxy-api, up to you.

I assume this will be a major release for devtools-protocol since it's probably a breaking change for some clients as files have moved.

Release notes on what changed would be nice too.

Thanks for grinding through this @brendankenny . Feels good to see puppeteer will be using something that started as an evening hack.

brendankenny · 2018-08-31T18:03:06Z

sorry for the pause here. I ran into a significant typescript performance issue with the approach in protocol-mapping.d.ts (microsoft/TypeScript#26756), but that's been fixed on master in microsoft/TypeScript#26790. Since this is a totally new file that won't affect existing users, I feel good moving forward with it even though it requires typescript@next for decent performance until 3.1 comes out.

Also happy to bikeshed on file naming. It might be a bit weird to call it protocol-api.d.ts since it's really an API for the protocol, not the API :)

I propose we call it protocol-rpc-api or protocol-proxy-api, up to you.

@paulirish thoughts on file naming?

Release notes on what changed would be nice too.

@paulirish on this too. Looks like the changelog is updated automatically. Should I just insert a note in there anyways or is there a better spot?

paulirish · 2018-08-31T18:26:12Z

naming

protocol-proxy-api sg.
domain-namespace-api is the closest collection of terms that I think of that fits, but i dont think its great.

changelog

changelog generation will overwrite it. that changelog is only about the protocol anyway, not the package. lets just put something the readme.

brendankenny · 2018-08-31T18:52:15Z

done

paulirish · 2018-08-31T19:54:47Z

published to devtools-protocol@0.0.588129

paulirish · 2018-08-31T19:55:31Z

thanks so much to @brendankenny for taking this on!

nojvek · 2018-08-31T19:55:43Z

README.md

@@ -6,5 +6,8 @@ Please [file issues](https://github.com/ChromeDevTools/devtools-protocol/issues)
 Use the [protocol viewer](https://chromedevtools.github.io/devtools-protocol/) for navigating the protocol. File issues at [its repo](https://github.com/ChromeDevTools/debugger-protocol-viewer) if you have a bug with the documentation webapp.


+TypeScript definitions for the protocol's types are available in ['types/protocol.d.ts'](https://github.com/ChromeDevTools/devtools-protocol/tree/master/types). Mappings from Commands and events to these types are available in either generated `DomainApi` style in [`types/protocol-proxy-api.d.ts`](https://github.com/ChromeDevTools/devtools-protocol/blob/master/types/protocol-proxy-api.d.ts) or in simple name-to-type-interface style in [`types/protocol-mapping.d.ts`](https://github.com/ChromeDevTools/devtools-protocol/blob/master/types/protocol-mapping.d.ts).


brendankenny commented Aug 13, 2018

View reviewed changes

paulirish reviewed Aug 13, 2018

View reviewed changes

brendankenny mentioned this pull request Aug 13, 2018

core(tsc): update types in emulation.js GoogleChrome/lighthouse#5830

Merged

brendankenny mentioned this pull request Aug 15, 2018

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

Merged

paulirish approved these changes Aug 15, 2018

View reviewed changes

nojvek reviewed Aug 15, 2018

View reviewed changes

nojvek approved these changes Aug 15, 2018

View reviewed changes

brendankenny added 8 commits August 31, 2018 10:56

update protocol-dts-generator to add command and event -> type mapping

9d5ab7a

feedback

e358fc4

also use tuple for event parameter

00039a2

includes('includes')

8895327

add command/event descriptions in file

7690b10

back to namespaces

9a2e919

add back ProtocolApi generation

ee1c489

update

f17edec

brendankenny force-pushed the newdts branch from 68f219d to f17edec Compare August 31, 2018 17:59

update name and readme

23066c6

paulirish merged commit 93338d7 into ChromeDevTools:master Aug 31, 2018

nojvek reviewed Aug 31, 2018

View reviewed changes

brendankenny deleted the newdts branch August 31, 2018 20:56

		@@ -6,5 +6,8 @@ Please [file issues](https://github.com/ChromeDevTools/devtools-protocol/issues)
		Use the [protocol viewer](https://chromedevtools.github.io/devtools-protocol/) for navigating the protocol. File issues at [its repo](https://github.com/ChromeDevTools/debugger-protocol-viewer) if you have a bug with the documentation webapp.


		TypeScript definitions for the protocol's types are available in ['types/protocol.d.ts'](https://github.com/ChromeDevTools/devtools-protocol/tree/master/types). Mappings from Commands and events to these types are available in either generated `DomainApi` style in [`types/protocol-proxy-api.d.ts`](https://github.com/ChromeDevTools/devtools-protocol/blob/master/types/protocol-proxy-api.d.ts) or in simple name-to-type-interface style in [`types/protocol-mapping.d.ts`](https://github.com/ChromeDevTools/devtools-protocol/blob/master/types/protocol-mapping.d.ts).

update protocol-dts-generator to add command and event to types mapping #113

update protocol-dts-generator to add command and event to types mapping #113

Conversation

brendankenny commented Aug 13, 2018 • edited Loading

brendankenny commented Aug 13, 2018

paulirish commented Aug 13, 2018

brendankenny Aug 13, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

brendankenny commented Aug 14, 2018

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

Choose a reason for hiding this comment

JoelEinbinder commented Aug 15, 2018

nojvek commented Aug 15, 2018 via email

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

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

Choose a reason for hiding this comment

nojvek left a comment

Choose a reason for hiding this comment

brendankenny commented Aug 16, 2018

nojvek commented Aug 17, 2018

brendankenny commented Aug 31, 2018

paulirish commented Aug 31, 2018

naming

changelog

brendankenny commented Aug 31, 2018

paulirish commented Aug 31, 2018

paulirish commented Aug 31, 2018

Choose a reason for hiding this comment

brendankenny commented Aug 13, 2018 •

edited

Loading

brendankenny Aug 13, 2018 •

edited

Loading

brendankenny Aug 13, 2018 •

edited

Loading