Since the logic is otherwise identical to SocketDebugAdapter, I would probably pull this logic into an abstract class NetworkDebugAdapter that has an abstract createConnection method that the Socket and NamedPipe debug adapters can implement.

Good call - refactored that to a NetworkDebugAdapter abstract class.

I meant to also move the rest of the logic within startSession() to the parent class, since the only thing different is the arg to net.createConnection 🙂

Oh yeah of course :) now I have an abstract createConnection that SocketDebugAdapter and NamedPipeDebugAdapter implement.

small tip: I usually just use crypto.randomBytes in cases like these. Slower but works just fine for tests.

Done.

Since this is a PR for an extension API request, we will have to go through the API review request, which starts by making a proposal in vscode.proposed.d.ts.
However, moving the DebugAdapterNamedPipeServer to vscode.proposed.d.ts does not allow to add that type to the OR-type DebugAdapterDescriptor in vscode.d.ts (which is missing in your PR anyway). So we have no choice but to leave the DebugAdapterNamedPipeServer in vscode.d.ts....

When looking at the resulting type:

export type DebugAdapterDescriptor = DebugAdapterExecutable | DebugAdapterServer | DebugAdapterNamedPipeServer | DebugAdapterInlineImplementation;

I got the impression that there is no real need to add a new type DebugAdapterNamedPipeServer because the name DebugAdapterServer covers the named pipe case nicely and we could just overload the constructor.
The only problem with this approach is the mandatory port variable which doesn't makes sense in the named pipe case...

Any ideas how to fold DebugAdapterNamedPipeServer into the DebugAdapterServer (in order to keep the API surface area small)?

I agree with you. I would love to merge the two (I wasn't quite sure what you might think of changing the existing definition...)

One thing we could do is have a type property (when no type is specified we assume socket:

	/**
	 * Represents a debug adapter running as a socket based server.
	 */
	export class DebugAdapterServer {
		/**
		* The type.
		*/
		readonly type?: 'socket' | 'namedPipe' | undefined
		/**
		* The port.
		*/
		readonly port?: number;
		/**
		 * The host.
		 */
		readonly host?: string;
		/**
		 * The named pipe path. We could reuse host for this?
		 */
		readonly path?: string;
		/**
		 * Create a description for a debug adapter running as a socket based server.
		 */
		constructor(port: number, host?: string);
		/**
		 * Create a description for a debug adapter running as a named pipe based server.
		 */
		constructor(path: string);
	}

I made port optional... but if we have to keep it mandatory (although I don't see how making something not mandatory anymore is breaking), we could assign port to -1 in the named pipe case.

What do you think?

The reason I suggested Tyler make a new type is so that the presence of host and port versus path is better typed. There should be no case where port is set but path is not, and no case host is set but port is not. I prefer to express cases like this in the type system when I can, but the expense is a more bulky API surface as you mention.

One way that this can be done is to make some endpoint: { path: string } | { host: string; port: string } property, but this is a breaking change and not necessarily much nicer.

@TylerLeonhardt if we make port optional, this breaks all code that assumes that this property always exists (because that code doesn't handle the undefined case). So we cannot make port optional.
But we could keep port mandatory and set it to 0 in the named pipe case... but this is somewhat ugly.

@connor4312 I fully agree with you on the "better typing". The only reason why I brought this up was the "better naming" of things: DebugAdapterServer is the "good name" that covers all servers, e.g. DebugAdapterSocketServer, DebugAdapterNamedPipeServer, DebugAdapterSmokeSignalServer... ;-)

With DebugAdapterSocketServer we get a new name without the corresponding peer DebugAdapterSocketServer.

But since we cannot solve the port issue in an elegant way, I think we should swallow the new type DebugAdapterNamedPipeServer.

@TylerLeonhardt could you please add DebugAdapterNamedPipeServer to the DebugAdapterDescriptor OR-type.
Since we cannot publish this new API to vscode.proposed.d.ts I suggest that we hold this PR back until it was discussed once in the API sync.

@weinand

How come we can't publish this new API to proposed? Is it because it needs to go into the OR-type of DebugAdapterDescriptor?

yes, if DebugAdapterNamedPipeServer is not part of DebugAdapterDescriptor it will be difficult to return an instance of it from DebugAdapterDescriptorFactory.createDebugAdapterDescriptor.