Command API – take two

[Reinmar]

tl;dr: It kinda seems that commands will become plugins and that editor -> command communication will be based on events which may allow improving the general code organisation and commands' public/private code organisation.

The setting

I needed to solve a few remaining issues with commands before starting writing any docs (commands will need to be covered even in high-level documentation). During a F2F discussion in the team, it turned out that @pjasiun has a significantly different view on the command API than what we implemented so far. Being conservative about this myself I had serious doubts because any conceptual change in this API means a lot of thinking (manual, careful refactoring). However, the idea sounds really good and we were able to imagine solutions to all problems (conceptual and technical) which we brought during the meeting.

But, let's start with some more details about what commands are.

• A command represents an action (like, "apply bold") and some state.
• Editor (even a concrete build) needs to have an open API which allows integrating (to some reasonable extent) external application with it. Right now, most of this would be done by commands – existing commands expose pretty much everything one would need to make UI for a headless editor (which boils down to creating some buttons and binding their state to our commands).
• Commands will be (together with plugins) the most utilised piece of API which contributors will use. Hence, they need to be a nice, easy to understand API.
• At the same time, commands are used as links between independent features. E.g. the autoformat plugin works by using other plugins' commands. It can do that without any specific knowledge about them. This strong dependency of commands' extensibility brings a bit of complexity to the topic (the refreshState event and private _doExecute()).

The issues that I initially noticed were:

• All commands implement _refreshState() method (protected, because it's only a callback to the refreshEvent) and public refreshValue() because most of the commands also have values but there's no special mechanism for this. See https://github.com/ckeditor/ckeditor5-core/issues/66.
• All commands also have _doExecute() and while it was meant to secure commands from being executed directly (they should be executed through editor.execute() which also checks the state and fires events (see https://github.com/ckeditor/ckeditor5-core/issues/22)). However, even we made the mistake to use _doExecute() in tests which made them slightly unrealistic. This was also a problem in CKE4 where many people skipped editor.executeCommand() and went straight to editor.getCommands( 'foo' ).execute().
• We should introduce command collection in order to easily perform command destruction and perhaps other actions like refreshing all commands on special events.

So, from one side we want to have a nice, consistent API, with no odd separation between private and public methods because this API will be used by thousands of developers. But at the same time, we want to secure it so it's used correctly. This was the starting point to the discussion.

Alternative proposal

@pjasiun proposed a slightly different approach towards commands which may solve the above issues and other kinds of problems which we encountered when implementing features.

The command API should be limited to "action" + "is enabled" (whether it can be executed). Value of the command and other pieces of its state (if it has any) should be implemented separately, in forms of plugins.

This approach has a number of interesting implications.

Command API can be simplified to the bare minimum (it will be much easier to secure a small API). So far, we wanted to enclose everything the command does inside that class. So e.g. every command constructor listens on specific editor/document events in order to refresh itself. By getting rid of the Command class we will need to move this code out to the plugin. We will also need to move command's value management out. This may sound like a lot of new code in already big plugins, but no one said that what currently was a command cannot now be a separate plugin.

So, we'll have plugins introducing commands. Like UndoCommandPlugin (to make it clear – I'm not saying that we need CommandPlugin). Such plugins will expose those pieces of the state which we removed together with the Command class. However, this will be optional – one will also be able to implement a command without defining a separate plugin which will be especially useful in closed applications.

Having plugins specialised in introducing commands means that we'll be able to have dependencies between those new command plugins. @pjasiun encountered an issue in the past that a command depended on another command but it could not enforce loading it like plugin can.

Similarly, we'll have no problems with destroying commands (detaching listeners they created) – all the listeners will be added by plugins.

We even started considering an approach in which "execute" and "refreshState" actions were also listeners. What would that cause? E.g. that unloading a plugin would remove all evidences of the command from the editor as well. This is very consistent with what I recently proposed regarding keystrokes. I even think that perhaps all listeners added by plugins (e.g. – conversion) should be added through the plugin so they'd be removed together with it. The downside is that one will not be able to check whether command was registered (although, one can check whether a plugin was loaded).

Cons?

The command is now a centralised "feature" instance. The plugin was understood by me more as a glue code, although the evidence are that many plugins contain quite a lot of logic. This has two implications.

I used to think of commands as the editor's external API. If one would like to create UI for the editor, without integrating with it much, commands would give them most of the information. Now this place will be gone.

Therefore, also building UI might get more complicated. Currently, UI components are bound straight to commands (button's isOn is bound to command's value and isEnabled to command's isEnabled).

To prevent that, we should move everything we have available on commands now to the plugins which will represent them. So a plugin would have isOn and value observable properties.

Extremum

As an extreme solution we could even consider whether commands are needed at all. If everything will be handled by plugins, then why do we need to register any "action" + "is enabled" pairs.

I feel that this may be too far, but it's worth investigating.

What's now?

The problem we're facing now is that we begun with valid issues for which the only clean solution we figured out mean moving a lot of code around. However, I don't see how I could write documentation now without clarifying this thing because commands will be one of the first things to explain.

Therefore, I plan to work on this topic. To come up with some prototypes, check how specific commands would need to be reimplemented and see if we don't miss anything. Then, we'll need a to see how we can roll out this change without blocking all the development.

Some prototype

This is written in 5 minutes so it's nothing definitive (especially, that I've hit a wall so it wouldn't work :D). Compare with https://github.com/ckeditor/ckeditor5-core/blob/919433775c502158ad04cc53fa1a22177397d0a6/src/command/toggleattributecommand.js. Note that pieces of this code can be replaced by a util – I wanted to show the mechanism rather than focus on code simplicity.

class BoldCommand extends Plugin {
	// Doesn't require anything (e.g. the BoldEngine) so one can use it with different converters
	// without any problems.

	constructor( editor ) {
		super( editor );

		this.set( 'isEnabled', false );
		this.set( 'value', false );

		this.listenTo( editor.document, 'changesDone', () => this.refresh() );

		// Register it as a command...
		// All the code below could be wrapped by a simple util:
		// editor.commands.register( 'bold', { execute, refresh } );

		this.listenTo( editor.commands, 'execute:bold', options => this._execute( options ) );
		this.listenTo( editor.commands, 'refresh:bold', ( evt, data ) => {
			data.isEnabled = this._checkEnabled();
		} );
		// Nooooo, I can't in any reasonable way set back `data.isEnabled` to `this.isEnabled`.
		// Unless someone can find some reasonably simple idea this may prove that relying on events in
		// 100% is not a good idea.
		// Edit: Perhaps the proposed above register() could do this if it knew the "this". It would be able
		// to set the `isEnabled` property. However, is it a good idea to have it set by an external class?
	}

	refresh() {
		// I don't like this bit either.

		this.commands.refresh( 'bold' );
		this.value = this._getValue();
	}

	_execute( options ) {
		// ... See https://github.com/ckeditor/ckeditor5-core/blob/919433775c502158ad04cc53fa1a22177397d0a6/src/command/toggleattributecommand.js#L94-L121
	}

	_checkEnabled() {
		const document = this.editor.document;

		return isAttributeAllowedInSelection( this.attributeKey, document.selection, document.schema );
	}

	_getValue() {
		return this.editor.document.selection.hasAttribute( this.attributeKey );
	}
}

Also, the CommandDispatcher which will be available in editor.commands:

class CommandDispatcher {
	constructor() {
		this.on( 'beforeExecute', ( evt, options ) => {
			this.fire( `execute:${ commandName }`, options );
		}, { priority: 'lowest' } );

		this.on( 'execute', ( evt, options ) => {
			this.fire( `afterExecute:${ commandName }`, options );
		}, { priority: 'lowest' } );
	}

	execute( commandName, option ) {
		this.fire( `beforeExecute:${ commandName }`, options );
	}

	refresh( commandName ) {
		const data = { isEnabled: true };

		this.fire( `refresh:${ commandName }`, data );
	}

	// It's missing some `isEnabled( commandName )` (the same story as the issues above).
}

I'll be working on this further. But I hope it's clearer now what's the proposed direction.

Related tickets

• https://github.com/ckeditor/ckeditor5-core/issues/66 – Improve typical command.
• https://github.com/ckeditor/ckeditor5-core/issues/22 – Command: afterExecute event (and possibly beforeExecute).
• https://github.com/ckeditor/ckeditor5-core/issues/14 – Move command helpers to a better place.
• https://github.com/ckeditor/ckeditor5-autoformat/issues/7 – InlineAutoformatEngine should work with commands, not attributes.

[Reinmar]

I realised after posting this that there are two big changes here:

1. Commands becoming plugins (or part of plugins) which improve several aspects like – dependencies, simpler commands implementation in closed apps, etc.
2. By organising editor -> commands communication through events we may improve several organisational aspects such as tear down process, consistency with private/public code, etc.

Anyway, I definitely need to work on this further to figure out the remaining issues.

[pjasiun]

// Nooooo, I can't in any reasonable way set back `data.isEnabled` to `this.isEnabled`.
// Unless someone can find some reasonably simple idea this may prove that relying on events in
// 100% is not a good idea.
// Edit: Perhaps the proposed above register() could do this if it knew the "this". It would be able
// to set the `isEnabled` property. However, is it a good idea to have it set by an external class?

All I can figure out right now is:

this.listenTo( editor.commands, 'refresh:bold', ( evt, data ) => {
	this.isEnabled = data.isEnabled;
}, { priority: 'LOWEST' } );

[fredck]

What I'm missing here is the clear interface signature that commands offer, especially when it comes to query its state. This is indeed one of the most common requirements when implementing a custom you based on buttons, when binding the button state to the tristate value of the command (on/off/disabled).

Same thing for state and execution events triggered by the command itself, which would now be triggered by editor.commands. Sounds a bit counter-intuitive. I mean, instead of dealing with a command directly, I'll have to proxy everything through editor.commands.

[pjasiun]

After a good while of thinking I get something like this:

class BoldCommand extends Plugin {
	// Doesn't require anything (e.g. the BoldEngine) so one can use it with different converters
	// without any problems.

	constructor( editor ) {
		super( editor );

		this.set( 'isEnabled', false );
		this.set( 'isBolded', false ); // a.k.a. value

		// Command need to implement observable `isEnabled` and `execute` and `refresh` method.
		editor.commands.register( 'bold', this );

		// `editor.commands` should call `refresh` on all commands on default events,
		// but this is how commands could add more events on which it should refresh:
		// 
		//		this.listenTo( doc, 'changesDone', () => this.refresh() );
		// 
		// Or to refresh all commands:
		// 
		//		this.listenTo( doc, 'changesDone', () => editor.commands.refresh() );

		refresh();
	}

	execute( options ) {
		// I think that method should be public and manually check if the command is enabled:
		if( !this.isEnabled ) {
			return;
		}

		// ...
		// See https://github.com/ckeditor/ckeditor5-core/blob/919433775c502158ad04cc53fa1a22177397d0a6/src/command/toggleattributecommand.js#L94-L121
	}

	refresh() {
		this.isEnabled = isAttributeAllowedInSelection( this.attributeKey, document.selection, document.schema );
		this.isBolded = this.doc.selection.hasAttribute( this.attributeKey );
	}
}

In this case, we could use the fact that isEnabled is obervable to implement the whole mechanism of blocking it, what was not possible in the CKEditor 4.

In this case, this is how I see read-only plugin:

class ReadonlyPlugin extends Plugin {
	init() {
		for( command of editor.commands ) {
			this.listenTo( command, 'change:isEnabled', () {
				command.isEnabled = false;
			}
		}
	}
}

Also, we were considering making it possible to prevent a change or overwrite the value of the observable values in the listener. I was against it and I still think that in most cases it's a very strange idea.

However, it this case it would prevent changing the value of isEnabled to true and back tofalse. So if we will introduce this mechanism the read-only plugin would do:

class ReadonlyPlugin extends Plugin {
	init() {
		for( command of editor.commands ) {
			this.listenTo( command, 'change:isEnabled', ( evt) {
				evt.stop();
			}
		}
	}
}

If we are scared of making all observables cancelable (I am) we may have it as an option to make only specific observable working this way: this.set( 'isEnabled', false, { cancelable: true } );. On the other hand, I think that if we will not overuse this mechanism we may try to introduce it (and I don't think we will since we did not have any use-cases for it yet).

The point is, we could achieve a very nice API using observable mechanism.

[Reinmar]

What I'm missing here is the clear interface signature that commands offer, especially when it comes to query its state. This is indeed one of the most common requirements when implementing a custom you based on buttons when binding the button state to the tristate value of the command (on/off/disabled).

That's true. In the case of commands, the public API must be really good. What makes things tricky is that it's also the private API what matters here because it also will be used by many developers.

We're definitely going to validate the final proposals for what API they create. Anything worse than what we have right now will not make sense.

Same thing for state and execution events triggered by the command itself, which would now be triggered by editor.commands. Sounds a bit counter-intuitive. I mean, instead of dealing with a command directly, I'll have to proxy everything through editor.commands.

It's the same pattern we use all across our code base – inversion of control based on event system. It was also present in the Command API before (the event-based refresh state logic) but it wasn't consistent. We use the same approach for conversion and all kinds of observers.

However, inverting the control in case of commands would indeed have no sense if the scope of what command is would not be changed. Right now a command is an entire class. What we're considering here is to make command an action which can be blocked when it cannot be executed in the current situation. There's no state here, no public API other than executing the command.

For example – user clicks a button. The UI library fires click event. Some other piece of code listens to that and executes the action. If a different piece of code wants to block that clicking action, it can listen to the same event and stop it.

Another example – conversion. A change is applied to the model and event for that change is fired. Plugins listen to those events and plug their converters.

Now, a piece of code decides that on some user action a command should be executed. The editor fires an event and some plugin handles it.

That would be all, but we also need to be able to implement the UI which requires to be bound to observable state properties (such as whether a feature is enabled and whether it's on). These, as @pjasiun noticed, are feature's properties. So, the question is how to implement them on the feature itself. And for that I don't have an answer yet.

The tricky thing here is that we'd like to invert the control of the commands "executability". However, it should be completely synchronised with the feature's property. That's what we need to figure out.

[Reinmar]

I quite like the @pjasiun's proposal. It's elegant and simple. Instead of inverting the control of command execution based on an event, it simply uses the strategy pattern just like we do it now. It also chooses to refresh the command in exactly the same way, which is a simplification comparing to what we have now.

However, I have mixed feelings regarding stretching observables in such a way. When we've been implementing observables we didn't even like that they used the event system. I'm more ok now with using the event system there (because it also solves some issues like removing observers), but we'd need to review the approach.

One, particularly inelegant, thing is that observable property's value can be controlled only by overriding it all the time. It creates some unclear situations. Peeerpahs, we could avoid that by defininig observable properties in such a way though:

get propName() {
    return this._observables.value;
}

set propName( newValue ) {
    const data = { value: newValue };

    this.fire( 'change:propName', data );

    this._observables.value = data.value;
}

But, it has a super ugly downside that obj.propName would still have the old value on obj#change:propName which is, I think, a no go.

In such a case, postfixing is the only option. Perhaps it's not that bad considering that we're used to postfix stuff in the engine, though :D.

[Reinmar]

I realised there's a problem with @pjasiun's proposal. If command's execute() method will be public, where will firing before/afterExecute events land? You can check in your command's execute() method whether it can be enabled but remembering that you also need to fire events would be irritating.

And that moves us back to square one. The only thing which is quite clear is that commands could usually be plugins (which register themselves to editor.commands). This will clean up the code a bit itself.

However, state resolution and editor -> command communication is still open.

[Reinmar]

Let's do a small recap.

• The command's state need to be queryable. Ideally, the command should be a retrievable object, so you don't need to do take its isEnabled value from the command collection and some other properties from the plugin collection (especially, that in both of these places the naming scheme will be different – 'Bold' plugin and 'bold' command).
• Implementing commands as plugins which register themselves will organise the code better and will make commands first citizens too.
• If we'd limit the command to execute + isEnabled (no value), then the action will always be the same – e.g. the bold command will always bold the text, never unbold (so most commands will go in pairs – style, unstyle; maximize, demiximize, etc.). This is what @pjasiun was insisting on, but even after a couple of days, I still don't like this idea. It cleans up the command a bit (it does just one thing), but it moves the mess out to the usage code. And while I usually like higher granularity, I don't think that it necessarily apply to commands too.
• If we go with public API only (like @pjasiun proposed) all the methods (i.e. execute(), refresh()) must be safe to call directly on the command. This means that every execute() will need to care about checking the sate, firing events and refresh() about making the state overridable. This would look clean but it will be inconvenient unless we'd find a way to avoid code duplication.
• If we go with public+protected API (like now) then we'll, of course, keep the status quo which is robust but a bit messy API.
• We can also try to mix both approaches and have public API only but with a better naming convention. E.g. the callbacks to editor events (refreshing and executions) will be exposed as onExecute() and onRefresh() which will tell the API user that they are not to be called directly. The really public API (execute() and refresh()) will then come from a mixin or a parent class and will be really usable.

[Reinmar]

We've been proceeding with @pjasiun's idea but on steroids.

Core

There is a simple CommandInterface:

/**
 * @module core/commandinterface
 */

/**
 * @interface CommandInterface
 */

/**
 * Flag indicating whether a command is enabled or disabled.
 * A disabled command should do nothing upon it's execution.
 *
 * @observable
 * @member {Boolean} #isEnabled
 */

/**
 * Executes the command.
 *
 * A command may accept parameters. They will be passed from {@link module:core/editor/editor~Editor#execute}
 * to the command.
 *
 * @method #execute
 */

/**
 * Refreshes the command.
 *
 * @method #refresh
 */

/**
 * TODO
 *
 * @event #execute
 */

And this is how the CommandCollection would look like:

/**
 * @module core/commandcollection
 */

export default class CommandCollection() {
	constructor( editor ) {
		this.editor = editor;

		this.listenTo( this.editor.document, 'changesDone', () => {
			this.refreshAll();
		} );
	}

	add( commandName, command ) {
		this._commands.set( commandName, command );
	}

	get( commandName ) {
		this._commands.get( commandName );
	}

	* names() {
		yield* this._commands.names();
	}

	execute( commandName, ...args ) {
		const command = this.get( commandName );

		if ( !command ) {
			/**
			 * Command does not exist.
			 *
			 * @error editor-command-not-found
			 * @param {String} commandName Name of the command.
			 */
			throw new CKEditorError( 'commandcollection-command-not-found: Command does not exist.', { commandName: commandName } );
		}

		command.execute( ...args );
	}

	refreshAll() {
		for ( const command of this._commands.values() ) {
			command.refresh();
		}
	}

	destroy() {
		this.stopListening();
	}
}

mix( CommandCollection, EmitterMixin );

Command examples

And I ported two commands to use it. One is a plugin:

/**
 * @module block-quote/blockquotecommand
 */

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import Position from '@ckeditor/ckeditor5-engine/src/model/position';
import Element from '@ckeditor/ckeditor5-engine/src/model/element';
import Range from '@ckeditor/ckeditor5-engine/src/model/range';
import first from '@ckeditor/ckeditor5-utils/src/first';

/**
 * The block quote command plugin.
 *
 * @implements module:core/commandinterface~CommandInterface
 * @extends module:core/plugin~Plugin
 */
export default class BlockQuoteCommand extends Plugin {
	/**
	 * @inheritDoc
	 */
	constructor( editor ) {
		super( editor );

		/**
		 * Flag indicating whether the command is active. It's on when the selection starts
		 * in a quoted block.
		 *
		 * @readonly
		 * @observable
		 * @member {Boolean} #value
		 */
		this.set( 'value', false );

		this.set( 'isEnabled', false );

		this.decorate( 'execute' );

		editor.commands.add( 'blockQuote', this );
	}

	/**
	 * @inheritDoc
	 */
	refresh() {
		this.value = this._getValue();
		this.isEnabled = this._checkEnabled();
	}

	/**
	 * Executes the command. When the command {@link #value is on}, then all block quotes within
	 * the selection will be removed. If it's off, then all selected blocks will be wrapped with
	 * a block quote.
	 *
	 * @fires execute
	 * @param {Object} [options] Options for executed command.
	 * @param {module:engine/model/batch~Batch} [options.batch] Batch to collect all the change steps.
	 * New batch will be created if this option is not set.
	 */
	execute( options = {} ) {
		if ( !this.isEnabled ) {
			return;
		}

                // ...
	}

	/**
	 * Checks the command's {@link #value}.
	 *
	 * @private
	 * @returns {Boolean} The current value.
	 */
	_getValue() {
            // ...
	}

	/**
	 * Checks whether the command can be enabled in the current context.
	 *
	 * @private
	 * @returns {Boolean} Whether the command should be enabled.
	 */
	_checkEnabled() {
            // ...
	}

        // ...
}

And one is a configurable class:

/**
 * @module heading/headingcommand
 */

import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
import mix from '@ckeditor/ckeditor5-utils/src/mix';

import Range from '@ckeditor/ckeditor5-engine/src/model/range';
import Selection from '@ckeditor/ckeditor5-engine/src/model/selection';
import Position from '@ckeditor/ckeditor5-engine/src/model/position';
import first from '@ckeditor/ckeditor5-utils/src/first';

/**
 * The heading command. It is used by the {@link module:heading/heading~Heading heading feature} to apply headings.
 *
 * @mixes module:utils/observablemixin~ObservableMixin
 * @implements module:core/commandinterface~CommandInterface
 */
export default class HeadingCommand {
	/**
	 * Creates an instance of the command.
	 *
	 * @param {module:core/editor/editor~Editor} editor Editor instance.
	 * @param {String} modelElement Name of the element which this command will apply in the model.
	 */
	constructor( editor, modelElement ) {
		/**
		 * The editor instance.
		 *
		 * @readonly
		 * @member {module:core/editor/editor~Editor}
		 */
		this.editor = editor;

		/**
		 * Unique identifier of the command, also element's name in the model.
		 * See {@link module:heading/heading~HeadingOption}.
		 *
		 * @readonly
		 * @member {String}
		 */
		this.modelElement = modelElement;

		/**
		 * Value of the command, indicating whether it is applied in the context
		 * of current {@link module:engine/model/document~Document#selection selection}.
		 *
		 * @readonly
		 * @observable
		 * @member {Boolean}
		 */
		this.set( 'value', false );

		this.set( 'isEnabled', false );

		this.decorate( 'execute' );
	}

	/**
	 * @inheritDoc
	 */
	refresh() {
		const block = first( this.editor.document.selection.getSelectedBlocks() );

		this.value = !!block && block.is( this.modelElement );
		this.isEnabled = !!block && checkCanBecomeHeading( block, this.modelElement, this.editor.document.schema );
	}

	/**
	 * Executes command.
	 *
	 * @fires execute
	 * @param {Object} [options] Options for executed command.
	 * @param {module:engine/model/batch~Batch} [options.batch] Batch to collect all the change steps.
	 * New batch will be created if this option is not set.
	 */
	execute( options = {} ) {
            // ...
	}

	destroy() {
		this.stopListening();
	}
}

mix( HeadingCommand, ObservableMixin );

---

The only code which is duplicated in both commands is this:

this.set( 'isEnabled', false );

this.decorate( 'execute' );

And a check for isEnabled in execute().

Other than that, a command needs to implement its refresh() and execute() but both are really straightforward.

Hooks

How did we implement the hook to control the isEnabled's value and execute event which will allow cancelling/modifying the command behaviour?

The first was already discussed above – we'd just use the change:isEnabled event. While not super clear, it's the simplest option and it's not going to be used very often anyway – it's for special cases.

The execute event is fired thanks to decorate( 'execute' ). The method decoration is explained in https://github.com/ckeditor/ckeditor5-utils/issues/162.

Other changes

We moved command refreshing to CommandCollection as 90% of commands need that. This has implications on #114 (comment). Destroying a plugin will not fully unload it unless a plugin does editor.commands.remove( 'myself' ) on its own destroy().

We allowed passing multiple arguments to execute().

Command plugins

That's a bit which didn't work as well as I hoped. At least one of our commands is configurable on runtime – HeadingCommand so it can't be a plugin. There are also other command classes now which accept additional params but these are not really configurable on runtime so we could still expose them as plugins.

However, I'm not sure if it won't be slightly confusing that one plugin is a command and another one is just a class which needs to be enabled by some other plugin... It's mostly about our code anyway because most developers, when implementing their plugins and commands, won't need to rely on runtime configuration so they will be able to implement commands as plugins.

[Reinmar]

I talked with @fredck and he expressed his concerns which confirmed my doubts. We agreed that:

• There should be #value in the CommandInterface. Most commands define it so it makes sense to standardise it.
• We shouldn't mix commands and plugins. This might help a bit but it mixes the concerns and makes things harder to reuse. We talked in the past that plugins should implement minimal logic and focus on being glue code and we should rather stick to that. If we'd be able to implement every kind of command or other feature as a plugin it might have had a sense. The plugin would become a container for any kind of feature. But we don't have such a system (and it doesn't make sense to design it at this point) and it failed short when we needed to have the flexibility to configure classes at runtime.
• Because of that we can bring back the Command class and put the duplicated code there. I've still have some doubts, though, regarding the changesDone listener and destroying commands. Since some command may plug some listener (using this.listenTo()) we should destroy them anyway which means that listening to changesDone can be moved to the command anyway.