Ckeditor5: Should first call of view.element render view.template?

[oskarwrobel]

This might cause confusing situations.

Using this.element inside view constructor (eg to initialize FocusTracker or KeystrokeHandler) renders the template and makes it not possible to be extended.

class MyView extends View {
    constructor( locale ) {
        super( locale );

        this.template = new Template( ... );

        this.focusTracker = new FocusTracker();
        this.fousTracker.add( this.element );
    }
}

const view = new MyView( locale );

Template.extend( view.template, { .... } );

And I see a warning:

template-extend-render: Attempting to extend a template which has already been rendered.

but my view is not even initialized.

To workaround this problem I can move FT init to the view init() but it took me a while to figure out why I can't extend this template.

[oleq]

The very first idea was that constructor defines but it's init does things related to rendering. Also, if not referenced, the view#element does not render, which supposed to be a kind of optimization against unnecessary execution and DOM manipulation.

When we first designed such approach, it looked right. TBH, today, I'm no so sure and it looks to me more like a premature optimization instead. It's confusing in cases like this one. Developers will struggle to understand the magic of view#element and it might not be the best DX.

What if we moved view.element = this.template.render() to View#init? It would mean that each class inheriting from View and implementing own init must first call super.init() in the method definition. I wonder if that would break something major.

WDYT? (cc @Reinmar)

[Reinmar]

There reason why we don't render the element in constructor() is to allow subclass existing view to change its template. That's the reason why this.element should not be accessed in the constructor(). Optimisation had nothing to do with that.

I completely agree that this is not the cleanest solution. Perhaps we could replace init() with render() so it would be clearer that until render() is called this.element is null and accessing it changes nothing. Less magic, more clarity. WDYT?

[oleq]

There reason why we don't render the element in constructor() is to allow subclass existing view to change its template. That's the reason why this.element should not be accessed in the constructor(). Optimisation had nothing to do with that.

I know, but instead of saying:

– Don't use #element in constructor if you want to subclass the #template.

we can simply say

– view#element is always null in constructor but it is rendered and accessible in init.

We don't need to rename init to render IMO, it doesn't change much in DX.

[Reinmar]

We don't need to rename init to render IMO, it doesn't change much in DX.

Depends on the semantics we want to have. What we do in init() now? Isn't this anything more than finishing rendering the view by adding missing listeners or something?

[oleq]

ATM View#init triggers recursive init of children and sets the #ready flag.

Particular classes inheriting from View do tons of stuff, including async initialization (see IframeView, still async after https://github.com/ckeditor/ckeditor5-ui/issues/225). I think it would be weird to write view.render().then( ... ). view.init().then( ... ) feels more precise because many things can happen at this stage and rendering is just one of them.

[Reinmar]

I don't think that we need anything besides render(). My idea was to replace the init() method with render() completely. Base View#render() would set view#element and you would normally override it to add more behavior.

[Reinmar]

Just look at this piece of docs:

Wouldn't render() better fit this domain?

[oleq]

I just fell for that one today. When adding view elements to FocusTracker/FocusCycler I didn't realise their templates are extended later on and I accidentally rendered them too early and broke some stuff added by the Template.

I think we should consider dropping this auto-rendering in the View#element getter. Although it warns in the console, it may be very hard to understand for the outside world anyway.

[Reinmar]

I marked this as 1.0.0 candidate.

[Reinmar]

Can lead to the removal of the init() method. All custom stuff will happen in render().

class MyView extends View {
     render() {
          const el = super.render();

          // Custom stuff.

          return el;

          // But what about `this.element`? Maybe returning makes no sense.
    }
}