Stabilize CustomExecution task API

[alexr00]

No description provided.

[alexr00]

@GabeDeBacker FYI

[GabeDeBacker]

One thing to note that I hit while consuming this proposed API which could be improved.

This onDidClose part of the PseudoTerminal interface is optional.

	interface Pseudoterminal {
		onDidClose?: Event<void | number>;

At some point while that API was in the proposed state, it was renamed. And since it was optional, the TypeScript compiler did not inform me I was doing something wrong. Also took me a while to figure out what the issue was.

I think it would be good to make sure developers really understand that in normal circumstances they should be firing the onDidClose event.

Since PseudoTerminal is already public, we could do this for CustomExecution

	 /**
	  * Make onDidClose required for tasks so developers realize they should
	  * implement this to have their tasks complete and it is enforced
	  * by the TypeScript compiler.
	  * If they want to have a task run forever (which is probably a small case),
	  * they simply don't fire the event.
	  */
	 export interface TaskPseudoterminal extends Pseudoterminal {
		onDidClose: Event<void | number>;
	}

	/**
	 * Class used to execute an extension callback as a task.
	 */
	export class CustomExecution2 {
		/**
		 * @param process The [Pseudoterminal](#Pseudoterminal) to be used by the task to display output.
		 * @param callback The callback that will be called when the task is started by a user.
		 */
		constructor(callback: () => Thenable<TaskPseudoterminal>);

		/**
		 * The callback used to execute the task. Cancellation should be handled using
		 * [Pseudoterminal.close](#Pseudoterminal.close). When the task is complete fire
		 * [Pseudoterminal.onDidClose](#PseudoterminalForTask.onDidClose).
		 */
		callback: () => Thenable<TaskPseudoterminal>;
	}

[GabeDeBacker]

One final note: If an extension fires onDidClose, will close get called?

Vice versa is also a question, if close is called, should onDidClose be called?

[alexr00]

@Tyriar for the intended behavior around close when onDidClose is called and the reverse.
The behavior I'm seeing is close is not called when onDidClose is fired and onDidClose isn't fired when close is called.

[alexr00]

Since we still have more to discuss for this API we'll continue in October. Leaving the Milestone as September for now though so we don't forget to keep discussing it.

[alexr00]

My thoughts on making onDidClose required for custom execution tasks is that, while it adds nice guidance, it also increase our API surface by basically duplicating an event. There are also cases where a task might never need to fire onDidClose (perhaps someone wants to have a task that never ends). I'll keep considering it and post an update with where we come out.

[Tyriar]

@Tyriar for the intended behavior around close when onDidClose is called and the reverse.
The behavior I'm seeing is close is not called when onDidClose is fired and onDidClose isn't fired when close is called.

This is by design, if you read close's comment carefully it hints at this:

Implement to handle when the terminal is closed by an act of the user.

close is called exclusively when the user somehow kills it, which you may want to handle differently to a regular close. You can always just call close from the onDidClose handler if not.

---

My thoughts on making onDidClose required for custom execution tasks is that, while it adds nice guidance, it also increase our API surface by basically duplicating an event.

👎 to making onDidClose mandatory just for tasks as it could be optional.

[bwateratmsft]

This will work for us, it will be a huge benefit. I would support coming up with some simplified examples somewhere as the CustomExecution+PseudoTerminal is significantly more complicated than ShellExecution/ProcessExecution are.

The only thing we really want in addition is #2809, an API to resolve task variables; but this can come later.

[alexr00]

The task provider extension sample has an example of how to use CustomExecution: https://github.com/microsoft/vscode-extension-samples/tree/master/task-provider-sample
Once we stabilize I will also add it to the documentation.