Remove Conversion#register()

[Reinmar]

Extracted from ckeditor/ckeditor5-engine#1639.

What's the point of Conversion#register() if for() returns only UpcastConversionsHelpers or DowncastConversionHelpers (according to its docs)? In fact, I'd love if we could somehow remove register() because realistically speaking it can be used together with new Conversion() only (just like it currently is). You can't register your own conversion helpers. We could make it possible to pass all helpers directly to new Conversion(). It's not critical, but I found it a bit weird and confusing how it's documented right now.

[jodator]

So the register() method is used to define the "upcast", "downcast", "editingDowncast" and "dataDowncast" dispatchers groups.

The Conversion on its own requires that "upcast" and "downcast" groups are defined (used in two-way-converters like editor.conversion.elmentToElement()) so this makes new Conversion() a broken instance. To make that beautiful we could introduce some builder:

const builder=  new ConversionBuilder();
builder.addDispatcher( 'downcast', this.data.downcastDispatcher, 'dataDowncast' );
builder.addDispatcher( 'downcast', this.editing.downcastDispatcher, 'editingDowncast' );
builder.addDispatcher( 'upcast', this.data.upcastDispatcher );

// This could check for required 'upcast'/'downcast'
this.conversion = builder.build();

but this too Java-ish probably.

Note: in above methods you only define dispatcher once with optional alias (ie for dataDowncast).

The other thing is we could leave this as is and refactor logic a bit:

const conversion = new Conversion();

conversion.addDispatcher( 'downcast', this.data.downcastDispatcher, 'dataDowncast' );
conversion.addDispatcher( 'downcast', this.editing.downcastDispatcher, 'editingDowncast' );
conversion.addDispatcher( 'upcast', this.data.upcastDispatcher );

or even more in line with the requirement for upcast/downcast:

conversion.addDowncastDispatcher( this.data.downcastDispatcher, 'dataDowncast' );
conversion.addDowncastDispatcher( this.editing.downcastDispatcher, 'editingDowncast' );
conversion.addUpcastDispatcher( this.data.upcastDispatcher );

as we require upcast/downcast dispatchers to be available in Conversion anyway.

Another solution would be pass everyghint to the constructor:

conversion = new Conversion( {
	upcast: this.data.upcastDispatcher,
	downcast: {
		dataDowncast: this.data.downcastDispatcher,
		editingDowncast: this.editing.dataDowncast
	}
} );

but is also ugly.

---

tl;dr - we cannot just remove .register() method. We must add some new API and I'd go with:

const conversion = new Conversion();

conversion.addDowncastDispatcher( this.data.downcastDispatcher, 'dataDowncast' );
conversion.addDowncastDispatcher( this.editing.downcastDispatcher, 'editingDowncast' );
conversion.addUpcastDispatcher( this.data.upcastDispatcher );

cc @pjasiun & @Reinmar, @scofalik (?)

[pjasiun]

Well... for me what we have now (register is fine, so its up to @Reinmar who was against it).

But some more ideas we had together with @jodator:

Simple names - values object:

this.conversion = new Conversion( {
	'downcast': new DowncastHelpers( [ this.editing.downcastDispatcher, this.data.downcastDispatcher ] ),
	'editingDowncast': new DowncastHelpers( this.editing.downcastDispatcher ),
	'dataDowncast', new DowncastHelpers( this.data.downcastDispatcher ),
	'upcast': new UpcastHelpers( this.data.upcastDispatcher )
} );

2 parameters downcast helpers and then upcast helpers (new DowncastHelpers and new UpcastHelpers can be omit):

this.conversion = new Conversion( {
		'downcast': [ this.editing.downcastDispatcher, this.data.downcastDispatcher ],
		'editingDowncast': this.editing.downcastDispatcher,
		'dataDowncast', this.data.downcastDispatcher
	}, {
		'upcast': this.data.upcastDispatcher
	} );

downcast and upcast as default keys:

this.conversion = new Conversion( {
		'editingDowncast': this.editing.downcastDispatcher,
		'dataDowncast', this.data.downcastDispatcher
	}, this.data.upcastDispatcher );

Downcast, upcast, then aliases:

this.conversion = new Conversion( [ this.editing.downcastDispatcher, this.data.downcastDispatcher ], this.data.upcastDispatcher, {
	'editingDowncast': this.editing.downcastDispatcher,
	'dataDowncast', this.data.downcastDispatcher 
} );

addAlias as a separate methods:

this.conversion = new Conversion( [ this.editing.downcastDispatcher, this.data.downcastDispatcher ], this.data.upcastDispatcher );
this.conversion.addAlias( 'editingDowncast', this.editing.downcastDispatcher );
this.conversion.addAlias( 'dataDowncast', this.data.downcastDispatcher  );

[pjasiun]

And I think that the last proposal (addAlias as a separate methods) is the best.