When producing types declarations, also emit the static types of the interfaces for TypeScript declarations. #284
Conversation
…interfaces for TypeScript declarations. This improves type checking on the declaration.
|
@roblourens what are your thoughts on this? I like the change overall, but it is breaking by introducing new type constraints. Do you know if there was a process for this kind of thing previously? |
adapter/src/protocol.ts
Outdated
| @@ -111,6 +111,8 @@ export class ProtocolServer extends ee.EventEmitter implements VSCodeDebugAdapte | |||
| this._pendingRequests.delete(response.request_seq); | |||
| clb(response); | |||
| } | |||
| } else if (msg.type === 'event') { | |||
| this._emitEvent(<DebugProtocol.Event>msg) | |||
There was a problem hiding this comment.
Not sure about this, it seems like the intent is not to emit DAP events but just errors and the like.
There was a problem hiding this comment.
The emitter is typed as emitting DebugProtocolMessage's, but that's just an empty interface 🤷 I think it'd make sense to emit them here
There was a problem hiding this comment.
But it's weird that it's also emitting non-DAP errors from the same emitter. But also that events currently aren't going anywhere? Do you have some more context on how this works at all currently @ashgti?
There was a problem hiding this comment.
I wasn't sure if there was a way to get DAP events if you used the adapter programmatically, but when I added the types here I noticed that events weren't being handled.
I can revert this part, it just seemed like a missing piece of the adapter.
There was a problem hiding this comment.
I think this method is only handling the data stream from the client, so this would only be hit if the client sends an event. I don't think that happens for any event types, or does it?
There was a problem hiding this comment.
It doesn't happen for any event types, removed.
| @@ -247,11 +248,7 @@ function property(name: string, optional: boolean, prop: P.PropertyType): string | |||
| s += comment(prop); | |||
| const type = propertyType(prop); | |||
| const propertyDef = `${name}${optional ? '?' : ''}: ${type}`; | |||
| if (type[0] === '\'' && type[type.length-1] === '\'' && type.indexOf('|') < 0) { | |||
There was a problem hiding this comment.
Sorry, can you give an example of what changed here?
There was a problem hiding this comment.
Oh, it's basically all of the type/event/command parameters. It's not clear to me why we are emitting these commented out. And does add a constraint but, for example, if you implemented one of the event types and set event different, then that would be wrong and the event wouldn't even be handled correctly. So I feel fine about that change.
There was a problem hiding this comment.
I think TypeScript will check that the event type (or type, etc.) matches the expected value if you where to try to use the wrong type.
|
Thanks, I'm good with this now |
When producing types declarations, also emit the static types of the interfaces for TypeScript declarations.
This improves type checking on the declaration.
Related to #283