Anonymous functions in constructors make extension more difficult (DebugSession)

[colin-grant-work]

Feature Description:

My particular interest at the moment is in the DebugSession class, but it is true more generally that if any anonymous function with a non-trivial implementation appears in a constructor, the whole constructor has to be rewritten if the class is extended and that listener is to be modified.

theia/packages/debug/src/browser/debug-session.tsx

Lines 77 to 112 in 5eb6efa

	this.connection.onRequest('runInTerminal', (request: DebugProtocol.RunInTerminalRequest) => this.runInTerminal(request));
	this.toDispose.pushAll([
	this.onDidChangeEmitter,
	this.onDidChangeBreakpointsEmitter,
	Disposable.create(() => {
	this.clearBreakpoints();
	this.doUpdateThreads([]);
	}),
	this.connection,
	this.on('initialized', () => this.configure()),
	this.on('breakpoint', ({ body }) => this.updateBreakpoint(body)),
	this.on('continued', ({ body: { allThreadsContinued, threadId } }) => {
	if (allThreadsContinued !== false) {
	this.clearThreads();
	} else {
	this.clearThread(threadId);
	}
	}),
	this.on('stopped', async ({ body }) => {
	// Update thread list
	await this.updateThreads(body);
	// Update current thread's frames immediately
	await this.updateFrames();
	}),
	this.on('thread', ({ body: { reason, threadId } }) => {
	if (reason === 'started') {
	this.scheduleUpdateThreads();
	} else if (reason === 'exited') {
	this.clearThread(threadId);
	}
	}),
	this.on('terminated', () => this.terminated = true),
	this.on('capabilities', event => this.updateCapabilities(event.body.capabilities)),
	this.breakpoints.onDidChangeMarkers(uri => this.updateBreakpoints({ uri, sourceModified: true }))
	]);

Here, if an adopter wants to modify the behavior of the class on a continued, stopped, thread, or terminated event, or access the full capabilities or breakpoint event in a custom implementation of updateBreakpoint() or updateCapabilities(), they have to rewrite the whole constructor.

Instead, it would be better to pass all of these through to named, overridable functions, e.g.

this.on('stopped', (e) => this.handleStopped(e))

The same is true for init functions, of course, and it might be worth codifying the avoidance of non-trivial anonymous functions in such contexts in the Coding Guidelines.

[akosyakov]

For such cases adopters should open a PR and make it more customizable. But forcing on everybody to extract a function because someone maybe somtimes wants to customize it is not a good reason to hold off PRs.

[colin-grant-work]

I'm not advocating that any particular PR be held up, only that good practice that doesn't hurt anything upstream and makes life easier for downstream projects should be considered for inclusion in the coding guidelines in the future.

[vince-fugnitto]

Closed by #8616.