Remove support for class based decorator

[springload]

Remove support for class based decorator

[thibaudcolas]

Thanks everyone for the feedback 😊

Initially I used classes because this project started as a port of a Ruby exporter, which uses classes. Then I tried to align with "what React does", but realised classes weren't actually necessary. They do have the advantage of allowing some sort of configuration of the decorators:

draftjs_exporter/example.py

Lines 53 to 66 in c963514

	class Link:
	def __init__(self, use_new_window=False):
	self.use_new_window = use_new_window
	def render(self, props):
	link_props = {
	'href': props['url'],
	}
	if self.use_new_window:
	link_props['target'] = '_blank'
	link_props['rel'] = 'noreferrer noopener'
	return DOM.create_element('a', link_props, props['children'])

... but this can also be implemented with function composition (although that's not really a Python idiom).

---

The "cost of supporting classes" is:

draftjs_exporter/draftjs_exporter/dom.py

Lines 79 to 89 in c963514

	if inspect.isclass(type_):
	# Class component, not instantiated.
	elt = type_().render(props)
	elif callable(getattr(type_, 'render', None)):
	# Class component, already instantiated.
	elt = type_.render(props)
	elif callable(type_):
	# Function component, via def or lambda.
	elt = type_(props)
	else:
	# Raw tag, as a string.

I don't know if there is much of a performance cost in this reflection code (would be easier to figure out once we tackled #65). Regardless, I don't think we will remove support for classes any time soon, but if we agree there isn't much of a point for them we'll update the documentation and example(s) to use functions defined in snake_case (then people can name them however they want in their own project), or also use classes.

If we do remove support altogether, it would be a 2.0.0 release, and presumably we would want to group together other breaking changes, but we want this project to be as stable and boring as possible so we'll see when we get there.

[su27]

It's ok for me. I don't think there is any obvious benefit to keep the class support. Maybe the only reason for class is semantical.

We use the term "decorator", which sounds functional in Python, but it's originally called Entity in draft.js, which is actually a React component. Maybe an "Entity" class with a render method, sounds more accurate to describe a component, than a function.

In Javascript, components defined by functions are normal and natural, like this TokenSpan. But in Python, it's a little strange, I prefer start-with-verb function name.(Ta-da! Verb war begins!)

But it's ok, not a big problem :)

[thibaudcolas]

I just realised while thinking about version 2 that classes are the only way to define composite decorators (for arbitrary text replacement).

I imagine these could be turned into dicts, following what Draft.js does (https://draftjs.org/docs/advanced-topics-decorators.html#compositedecorator), a pattern somewhat similar to that of the block definitions:

{
    // A function in Draft.js (using a regex or anything else) – could just be the regex for the exporter.
    strategy: hashtagStrategy,
    // A React component in Draft.js – Python function in our case.
    // Should be called `element` if we followed the same pattern as the block definitions and also supported using tags directly.
    component: HashtagSpan,
},

This is a bit more work than just removing those conditionals in DOM.render.

Edit: ah actually it's possible to patch the composite decorators implementation so this part of the API keeps the class definition if we want to limit the rework. In:

draftjs_exporter/draftjs_exporter/composite_decorators.py

Lines 32 to 35 in 570b3ac

	yield DOM.create_element(decorator, {
	'match': match,
	'block': {
	'type': block['type'],

change decorator to a lambda: lambda props: decorator.render(None, props)

---

Oh and in terms of performance, using the new benchmark I see a speed-up of about 5% when removing support for classes (literally every block, style, entity, piece of text goes through these DOM.render conditionals, and the checks are ordered from least to most common).

[loicteixeira]

Pinging @everyonesdesign @su27 as you guys seem to have used the project a fair bit.

[everyonesdesign]

Yes, it's a long time since I worked with code using draftjs_exporter, but I remember I thought about that.

I didn't have any cases for classes usage here, and I could do everything I've done with functions components.

So, if there're no known cases for classes usage here, I think it's a good idea to drop them and use functions. It's a win for code readability.

[loicteixeira]

I don't think the performance cost of supporting classes is that high (but proper numbers would be good).

However we can already see it won't work with the Link class example because then, it would need to pass the right parameters to the constructor depending on what the class' __init__ accepts (unless it has **kwargs so the signature is always the same, but then you have to somehow have the props on one side and some additional kwargs on the other hand).

I can agree with leaving it off until the 2.0.0 release (I had forgotten we reached 1.0 already), maybe by then we will have a better idea whether classes are useful. In the meantime, updating the documentation is a good idea.

Thanks for the feedback everyone.