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
[chrome-remote-interface] add support for shorthand callback version of events #63570
[chrome-remote-interface] add support for shorthand callback version of events #63570
Conversation
@kazarmy Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 63570,
"author": "kazarmy",
"headCommitOid": "f35cec38aa064af255cf94b7475a10df8f946a1e",
"mergeBaseOid": "b527ff5063897a5947d5fc0aa4ac5e549c8791a9",
"lastPushDate": "2022-12-11T08:40:06.000Z",
"lastActivityDate": "2022-12-11T14:38:45.000Z",
"mergeOfferDate": "2022-12-11T14:37:51.000Z",
"mergeRequestDate": "2022-12-11T14:38:45.000Z",
"mergeRequestUser": "kazarmy",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "chrome-remote-interface",
"kind": "edit",
"files": [
{
"path": "types/chrome-remote-interface/chrome-remote-interface-tests.ts",
"kind": "test"
},
{
"path": "types/chrome-remote-interface/index.d.ts",
"kind": "definition"
}
],
"owners": [
"kazarmy",
"westy92"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "westy92",
"date": "2022-12-11T14:37:07.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 1345460484,
"ciResult": "pass"
} |
🔔 @westy92 — please review this PR in the next few days. Be sure to explicitly select |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very impressive! 🎉
The missing JSDocs are fine, at least for now.
Could @borrows
(https://jsdoc.app/tags-borrows.html) be helpful here?
@westy92 Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
The problem is that I can't do: /**
* @borrows `${D}.${E}` as `${E}`
*/
[event in GetEvent<D>]: |
Thanks! 🎉 |
Ready to merge |
Regarding the JSDoc problem, another option is to use key remapping, say: type DoEventProps<D extends string> = {
[event in keyof ProtocolMappingApi.Events as GetEventFromString<D, event>]:
(listener: (params: GetReturnType<D, GetEventFromString<D, event>>, sessionId?: string) => void) => () => Client}; but microsoft/TypeScript#50715 indicates that JSDoc documentation isn't currently transferred over, so I suppose the JSDoc problem will have to be put on hold for now. |
Please fill in this template.
npm test <package to test>
.If changing an existing definition:
This pr adds support for statements of the form:
as shorthand for
Unfortunately there doesn't seem to be a way to copy over associated JSDoc, but they can be retrieved easily (with slight inconvenience) using the second form above, e.g. by typing in
and then hovering the mouse over the partial statement.