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

protocol-dts-generator for devtools-protocol typescript types #90

Merged
merged 7 commits into from
Mar 30, 2018
Merged

Conversation

nojvek
Copy link
Contributor

@nojvek nojvek commented Mar 23, 2018

Fixes #55 (typescript definitions)

TODO List

@nojvek
Copy link
Contributor Author

nojvek commented Mar 23, 2018

@paulirish ^

@nojvek nojvek changed the title v1 of protocol-dts-generator protocol-dts-generator for devtools-protocol typescript types Mar 23, 2018
@JoelEinbinder
Copy link

This is very cool. I tried to use it to add types to puppeteer and ran into trouble with events. Having a separate method per event makes it difficult to work with. Id expect the domains to extend Node's EventEmitter and have events defined like

on(event: 'TargetInfoChanged', listener: (params: Target.TargetInfoChangedEvent) => void): this;

Also every method is marked as optional on every domain. Maybe this should correspond to whether or not the method is experimental? Or have some other indication of experimental vs stable methods.

@nojvek
Copy link
Contributor Author

nojvek commented Mar 23, 2018

@JoelEinbinder done.

Yeah I left it as onXYZ because typescript would give better code completion, but they've fixed that since then.

Happy to take feedback on what you'd like the ideal interface to be.

do you simply want the interface to be .

send(command: 'Domain.xyz', params: SomeRequest): Promise<SomeResponse>
on(event: 'Domain.xyz', listener: (params: SomeEvent) => void): void

@@ -0,0 +1,228 @@
import * as fs from 'fs'
import * as path from 'path'
import {IProtocol, Protocol as P} from './protocol'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protocol-schema, now, eh?

@brendankenny
Copy link
Contributor

FWIW I've found the overloaded on() doesn't play well with using these types from js (using jsdocs), but OTOH it is the best way to get type inferencing from the compiler (to avoid given manual types to event responses). I'm playing with a different approach for Lighthouse since we have places that deal with consuming and producing (or at least re-playing) protocol methods. If I get it working I can make a proposal to combine it with this work so both options are supported.

All the info we need is in here, though, and as long as all the method definitions are on DomainApi or whatever (like you have now), we're good.

@nojvek
Copy link
Contributor Author

nojvek commented Mar 26, 2018

@JoelEinbinder @brendankenny Please let me know your preferences for the final interface and I'll make the changes.

Copy link
Contributor

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the protocol format you have now LGTM

@paulirish
Copy link
Member

@JoelEinbinder you happy with the look of this now?

@paulirish
Copy link
Member

@nojvek the remaining TODOs are some good stuff. do you want to do those in this PR or the next one? Ideally we wouldn't delay it too much.

@nojvek
Copy link
Contributor Author

nojvek commented Mar 27, 2018

I’m happy it being a v1 and work on documentaion and protocol steps in next PR

How does the publish of npm modules work? I imagine some bot runs the script?

@paulirish paulirish merged commit e367b98 into ChromeDevTools:master Mar 30, 2018
@paulirish
Copy link
Member

@nojvek Yup. Done by a bot every hour.

@themaxdavitt
Copy link

I created a PR to ensure the *_protocol.json files are typechecked with protocol-schema.d.ts (from the TODO list): #211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FAQ
5 participants