Expose all conversion utilities

[Reinmar]

A sibling ticket of https://github.com/ckeditor/ckeditor5-engine/issues/1555.

We exposed half of conversion utilities as editor.conversion's methods. What we miss are the methods from:

• downcast-converters
• upcast-converters

Some of those functions are needed quite frequently which we experienced when answering to various questions. I think that in most questions about conversion we needed some of these lower-level functions rather than those big factories we already have exposed.

I can see that there are quite a lot of downcast-converters but I'd expose all of them anyway, without a research which of them are used and how often (half of them are used in quite advanced cases with markers, but we'll want to make markers more popular anyway).

I'd expose this functions as e.g.:

editor.conversion.downcastConverters.downcastElementToElement()
editor.conversion.upcastConverters.upcastElementToElement()

[pjasiun]

Maybe?

editor.conversion.downcast.elementToElement()
editor.conversion.upcast.elementToElement()

[pjasiun]

Or just:

editor.conversion.downcastElementToElement()
editor.conversion.upcastElementToElement()
```

[pjasiun]

I can see that there are quite a lot of downcast-converters

Note that this is because we still do not close https://github.com/ckeditor/ckeditor5-engine/issues/1306.

[Reinmar]

Note that this is because we still do not close #4281.

I'm actually unsure now about hiding them :D With time, I got the feeling that most questions about conversion are questions about some odd cases. Like, extending existing converters, overriding some things, etc. I can't tell from my experience which of the functions we need, but if these helpers are used in our converters, there's a high chance that they can help in people's scenarios.

[pjasiun]

if these helpers are used in our converters, there's a high chance that they can help in people's scenarios.

It's not exactly this that, that we have helpers and then use them in converters. It is more like: we have converters and then part of these converters is exported besides these converters as the whole. It is not like wrap is a flexible until and there are are converters using it. It is just a part of downcastAttributeToElement designed as a listener attached by this function. Note that there is a 1-1 connection between converters and "utils", they are not reusable between converters.

[pjasiun]

I do not mean that these parts of converters will be useless, but they might be very hard to document them in a clear way. I think that you can more-less easily explain what converters and writer are, but it will be hard to tell how to use this "helpers" properly (btw we used to call them converters in the past). It was easier before the previous refactoring, but then we introduce one more level (called "converters" today).

[Reinmar]

Hm, ok. I'll use your list from https://github.com/ckeditor/ckeditor5-engine/issues/1306#issuecomment-378870980.

The public ones will be available via conversion.upcast/downcast. The protected ones will be still exported but documented as protected (I should also prepend their names with _). To avoid confusion and duplication in the API docs, I'd stop exporting the public ones – they'd be only available in conversion.upcast/downcast. WDYT?

[pjasiun]

👍

You can also skip insertText and remove since they have a very specific, hardcoded usage. The rest should be useful.

[Reinmar]

Hm... there's a problem with the naming. Look at the code I wrote in https://github.com/ckeditor/ckeditor5-table/issues/122#issuecomment-424100223.

After the API change I'd write this:

editor.conversion.upcast.attributeToAttribute( {
    // ..
} );

And this wouldn't work as well ;| The inconsistency in this looks very bad:

// You need to use for().add()...
editor.conversion.for( 'upcast' ).add(
	editor.conversion.upcast.attributeToAttribute( {
		// ...
	} )
);

// But this time you don't...
editor.conversion.elementToElement( {
	// ...
} );

Do you have any ideas how we could straighten this up?

[Reinmar]

One thing would be to be careful in the documentation and always use editor.conversion.for().add() when necessary. But this still will look bad in the code. We may try with a bit extended naming like:

editor.conversion.upcast.getAttributeToAttributeConverter()
// or...
editor.conversion.upcast.createAttributeToAttributeConverter()

Which indicatest that this is a getter/factory. But this way we'll make the names awfully long so it'd be good to find a better option.

The second problem I have is that editor.conversion.upcast... sounds like "let's do something" cause I read "upcast" as a verb. We wanted this to be a namespace, but I think this is risky.

[Reinmar]

One thing I'm sure is that we shouldn't mix those converter factories (which require to be used in for().add() in the editor.conversion.* object. That's because we have those two-way converters there already so mixing them together would be confusing too.

Another thing that I didn't like in the code samples I show above is the length of them. This looks sad:

editor.conversion.for( 'upcast' ).add(
	editor.conversion.upcast.attributeToAttribute( {
		// ...
	} )
);

What if... for() would return an object with more methods than just add()?

editor.conversion.for( 'upcast' ).attributeToAttribute( {
    // ...
} );

This way:

• we keep this maximally short,
• you cannot use this method in a wrong way simply because you don't need to do anything more than this.

[jodator]

What with dataDowncast & editingDowncast in the downcast pipeline?

I like the editor.conversion.downcast.elementToElement() format more then

editor.conversion.for( 'upcast' ).add(
	editor.conversion.upcast.attributeToAttribute( {
		// ... too much and confusing :/
	} )
);

as it is consistent with foo.bar.conversion.elementToElement() helpers..

So then we should also expose them:

editor.conversion.downcast.elementToElement()
editor.conversion.downcast.data.elementToElement()
editor.conversion.downcast.editing.elementToElement()
// or
editor.conversion.downcast.elementToElement()
editor.conversion.dataDowncast.elementToElement()
editor.conversion.editingDowncast.elementToElement()

[jodator]

All below proposition will take the simples custom converter for image - using 'dataDowncast' pipeline:

import {
	downcastElementToElement
} from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';

editor.conversion.for( 'dataDowncast' ).add( downcastElementToElement( {
    model: 'image',
    view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
} ) );

I: initial proposition (updated)

// ver I a.
editor.conversion.for( 'dataDowncast' )
    .add( editor.conversion.createDowncastElementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } ) );

// ver I b.
editor.conversion.for( 'dataDowncast' )
    .add( editor.conversion.downcast.createElementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } ) );

Comments:

• lengthy method names & code bloat

II: (PK's proposal) expose all converters in .for()

editor.conversion.for( 'dataDowncast' ).downcastElementToElement( {
    model: 'image',
    view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
} );

// Problem: nothing prevents this ¯\_(ツ)_/¯
editor.conversion.for( 'upcast' ).downcastElementToElement( {
    model: 'image',
    view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
} );

Comments:

• either exposes all (:-1:) or requires conversion group to explicitly add helpers (check below)
• Slightly bloated docs? (or not - check below)
• Dunno if we wanna keep them to be chainable (as with .add()?

III: Prop: explicit helper chain (kinda meh but I'll post it anyway)

editor.conversion.for( 'dataDowncast' )
    .addDowncastHelper()
    .elementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } );

editor.conversion.for( 'upcast' )
    .addUpcastHelper()
    .elementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } );

// But also:
editor.conversion.for( 'downcast' )
    .addUpcastHelper()
    .elementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } );

Comments:

• not chainable - or it will be fugly
• too explicit?

---

I would go with PK's (II) proposal as it looks the best but I'd limit available helpers returned from .for() for upcast/downcast only.
It would require defining them in different way though:

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

// Will require to expose API for proper upcast/downcast
this.conversion.register(
    'downcast',
    [ this.editing.downcastDispatcher, this.data.downcastDispatcher ],
    { useDowncastHelpers: true }
);
this.conversion.register(
    'upcast',
    [ this.data.upcastDispatcher ],
    { useUpcastHelpers: true }
);

// Or
this.conversion.register(
    'upcast',
    [ this.data.upcastDispatcher ],
    { helpers: UpcastHelpers }  // or new UpcastHelpers...
);

[scofalik]

I am for PKs proposal. Three things.

1. I don't mind that it is possible to add an upcast converter to the downcast pipeline. It is possible at the moment, so this would not be any kind of regression. You can do a lot of things incorrectly when coding and we cannot and should not secure them all. 🤷‍♂️
2. Remember to still keep .add() method as it can be used for custom converters.
3. I think chaining is a tertiary feature so I wouldn't care about it.

[jodator]

After F2F talks @pjasiun & @scofalik we agreed on PKs proposal with defining which helpers goes for which pipeline. Also due to fact of increased numbers of parameters the conversion.register() will accept an object:

this.conversion.register( {
    name: 'downcast',
    dispatchers: [ this.editing.downcastDispatcher, this.data.downcastDispatcher ],
    helpers: DowncastHelpers
} );

There will be two objects (interfaces) which will group proper helpers (DowncastHelpers & UpcastHelpers) - the name can be discussed during PR as now I don't have final name proposition.

The chaining is not required.

[pjasiun]

Agree with @jodator and @scofalik (and PK) that proposal II, with registering helpers for each pipeline, looks the best. I would go with:

this.conversion.register( {
    name: 'upcast',
    dispatcher: this.data.upcastDispatcher, //acepts also array dispatcher: [ this.editing.downcastDispatcher, this.data.downcastDispatcher ]
    helpers: UpcastHelpers // or upcastHelpers
);

Then this.conversion.for( 'upcast' ) returns upcastHelpers so you can do:

this.conversion.for( 'upcast' ).elementToElement

[jodator]

🏆 first

[pjasiun]

By the way it will be nice that you will be able to do:

this.conversion.elementToElement
this.conversion.for( 'dataDowncast' ).elementToElement
this.conversion.for( 'upcast' ).elementToElement

And each of these will execute the proper method.