marionettejs / backbone.marionette

The Backbone Framework
https://marionettejs.com
Other
7.07k stars 1.26k forks source link

Migrate view layer from Backbone.View to CustomElement/WebComponent #3573

Open blikblum opened 6 years ago

blikblum commented 6 years ago

Custom Elements are gaining traction as the best way to create interoperable components for web. Ionic, the popular mobile framework, is being rewritten with custom elements. Angular is making Custom Element a first class citizen.

By migrating to Custom Element, Marionette would align with current web development technology and opens the door to easier interoperability. The code would be simplified a lot since Custom Elements already implement features like attach/detach(connect/disconnect) lifecycle events.

Implementation could be done with class mixins like is skatejs and elix are doing.

SkateJS allows to customize the rendering with a similar concept as Marionette setRenderer, using mixins instead of a class method.

Marionette would provide withEvents, withUI, withBehaviors etc mixins with each functionality implemented and a withView mixin with all features. This would allow to customize the View class as needed.

The default one should be:

import {withView} from 'marionette'

const View = withView()

if only events is used the user could do


import {withEvents} from 'marionette'

const SimpleView = withEvents()

Is has some drawbacks like being a big breaking

Pros

Cons

paulfalgout commented 6 years ago

My first thoughts are that this would probably need to be done over multiple major releases.

First I think removing Backbone as a dependency is a good idea.

I also think it'd be nice if the pieces could be created so that we could continue supporting the current API / Backbone.View while allow with a plugin or a NextView or something that could potentially handle Custom Elements. I am a bit concerned that Custom Elements as the only solution would really restrict our compatibility with older browsers which has always been strongly supported by Mn. Would this require that Marionette need babel or a polyfill to run outside of Chrome?

I'm also assuming we could essentially provide the class mixins for direct use but also provide the class that mixes in the defaults (at least for a while) to keep close similarities to the current API?

essentially:

const View = withView();
export { View }
blikblum commented 6 years ago

Would this require that Marionette need babel or a polyfill to run outside of Chrome?

Yes. Babel to support ES6 class and polyfill for Web Components

I'm also assuming we could essentially provide the class mixins for direct use but also provide the class that mixes in the defaults (at least for a while) to keep close similarities to the current API?

Yes

JSteunou commented 6 years ago

I'm 100% on the feeling Marionette should move toward this direction. I'm actually tweaking a View to use JSX and VDom with snabbdom. I made it work but it just feels wrong because there is so much unneeded things for this case, inherited from Backbone concepts... I reached the limit of the setRenderer and composing it's own class like skatejs does to make it's own Marionette View would be a nice 1st step. The second step would be make every Marionette View a web component.

blikblum commented 6 years ago

JSX and VDom with snabbdom

BTW: i'm following the same route (after a short time using idom)

I made it work but it just feels wrong because there is so much unneeded things for this case, inherited from Backbone concepts...

Also inherits from string renderer concepts like dom:refresh and dom:remove events

paulfalgout commented 6 years ago

I'm all for moving the lib towards more modern trends, but what we can't do is require large rewrites or removal of the string templating in order to do so. What are the smaller steps to improving setRenderer and/or making the lifecycle events dependent on the type or renderer or whatever? I'm much more interested in the small step that makes supporting JSX and snabbdom more accessible over replacing view with an entirely different API. Though it does seem like some sort of mixin solution could cover this

JSteunou commented 6 years ago

Maybe @blikblum and I could work on a View concept as the final goal to get to and then split it up step by step to get a better overview. What do you think ?

blikblum commented 6 years ago

Though it does seem like some sort of mixin solution could cover this

Yes.

Instead of a simple function the renderer should be an object with a few functions like the domApi

From top of my mind it should control if reinitialize regions at each render, if trigger dom:remove, dom:refresh

paulfalgout commented 6 years ago

I don't entirely understand the dom:refresh issue. How does VDom change anything in a case where what is in the dom changes:

const MyView = View.extend({
  getTemplate() {
    // Similar logic could easily be done in the template itself, but this is a dramatic example
    if(this.model.get('foo')) return FooTemplate;
    return BarTemplate;
  },
  ui: {
    datepicker: '.datepicker'
  },
  onDomRefresh() {
    this.ui.datepicker.datepicker();
  },
  onDomRemove() {
    this.ui.datepicker.cleanUpPlugin();
  }
});
blikblum commented 6 years ago

How does VDom change anything in a case where what is in the dom changes:

With VDom, there's no way to know when the dom will change at View level.

Take a real example (template is jsx, state is the serialized model):

export default Marionette.View.extend({
  events: {
     'input input': 'updateModel'
  },

  modelEvents: {
    change: 'render'
  },

  template(state) {
  return (
  <div>
    <h3>{state.name}</h3>         
    <input type="text" name="name" className="form-control" value={state.name}/>
    <input type="checkbox" name="showDatePicker" className="form-control" value={state.showDatePicker}/>
    {state.showDatePicker && <div id="my-date-picker"/>}
  </div>
  )
  },

  updateModel(e) {
    let data = Backbone.Syphon.serialize(this)
    this.model.set(data)
  }
})

Every time an input event occurs, the view is re-rendered.

If state.showDatePicker is true the datepicker is show (dom element inserted), otherwise the dom element is removed

When the name input changes, there's no change to datepicker element but dom:remove and dom:refresh will always be triggered making not suited to initialize , e.g., a jQuery plugin.

There are two ways of initializing/destroying a jQuery plugin with VDom: manually check at each render like here or use the VDom specific features. In case of Snabbdom it provides hooks for dom mutation

JSteunou commented 6 years ago

What do you think of a 1st step being a rewrite of render and setRenderer logic being something like this:

render() {
    if (this._isDestroyed) { return this; }

    this.triggerMethod('before:render', this);

    if (this.shouldSetElement()) {
        this._setElement();
    }

    if (this.shouldReinitRegion()) {
        this._reIniRegion();
    }

    let el;
    if (this.shouldRenderTemplate()) {
        el = this.attachElContent(this._renderTemplate());
    }

    this.bindUIElements();

    this._isRendered = true;

    this.triggerMethod('render', this);

    return el;
}

A lot of changes here, let me explain step by step:

shouldSetElement

1st big breaking change that would involve ditch Backbone.View to put all inside Marionette.View. The issue of Backbone.View is that setElement is called too soon and have an opinion about View must have a root element before render, separated from template possible root element. I always struggle with Backbone.View because this.el, bindings, ui are set at init instead of render.

I propose that the root element, when having one, should only be set at (first) render. A default string template implementation matching the actual behavior could be something like

shouldSetElement() {
    return !this._isRendered;
},

_setElement() {
    // same logic than actual, checking el, tagName, etc.
}

The advantage of this API is that someone can keep string templates like handlebars, but with the view.el matching the template root node by writing a renderer like this

shouldSetElement() {
    return false;
},
attachElContent(el) {
    this.el = el; // naive setElement bindings should be reset
    return el;
}

Or having a renderer dealing with VDOM

shouldSetElement() {
    return false;
},
attachElContent(vnode) {
    if (!this.el) {
        this.el = document.createElement(vnode.sel);
    }
    this._vel = patch(this._vel || this.el, vnode);
    return this._vel;
}

You could even return false because you want to have your own logic

souldSetElement() {
    // let me handle this
    return false;
}

shouldReInitRegion

Solve the should we or not re-init the regions at re-render case. Default renderer could just return true. Others from the diff and patch family should return false. Plus, like above, it adds a ways to provide more logic before returning false.

shouldRenderTemplate

This one might seems useless, but I like being consistent and it might open some doors to a thing like shouldComponentUpdate thing. Plus, again, it allows people to add own logic if needed before returning true or false.

The biggest change is more the attachElContent out of renderTemplate. Easier to override and to control what to return.

return value

Should return the node or vnode to be able to use View as a component. A lot of library allow view as function or Class if they have a render method and this one return a node or vnode. This would ease the writing of a JSX + VDOM renderer and this renderer solve all regions issues thanks to this kind of syntax.

template() {
    // I dont like the CapitalCase but needed for JSX custom elements
    const SomeChild = new ChildView();
    return <div><h1>title</h1><SomeChild></SomeChild></div>;
}

renderer

A default renderer matching the actual this.el as root node + string template could be:

shouldSetElement() {
    return !this._isRendered;
},
shouldReinitRegion() {
    return true;
},
shouldRenderTemplate() {
    return true;
},
attachElContent(html) {
    this.Dom.setContents(this.el, html, this.$el);
    return this.el;
},

Then community and / or Marionette team could provide more renderer for people to use and even mix with others renderer so you could do

import {jsxVdom} from 'marionette/renderers';

export const VDomView = Marionette.View.setRenderer(jsxVdom);

There is a lot to think and solve above this, but I think this is a start.

paulfalgout commented 6 years ago

I am rather against adding public API to the view.

Ideally this is done from the renderer entirely.

However I don't really see calling _setElement as an option. That's going to be very leaky. Anything using this.el or this.$el will be in trouble.

blikblum commented 6 years ago

In any case, the changes should be made post v4

paulfalgout commented 6 years ago

I agree, though.. should the renderer API be left experimental through v4 allowing for some breaking changes? It would only be breaking of 3rd party renderers.

blikblum commented 6 years ago

I agree, though.. should the renderer API be left experimental through v4 allowing for some breaking changes?

I think so

JSteunou commented 6 years ago

I am rather against adding public API to the view.

I think it's doable to put all the logic inside a renderer

However I don't really see calling _setElement as an option.

It is a non real option because anyone returning always false instead of !this._isRendered should have the responsibility to set a this.el on it's own, and API details are to be discussed this is just an idea to show a possible way.

In any case, the changes should be made post v4

We all are on the same page on this one :)

blikblum commented 6 years ago

For the record. Link of a nice article about we components: http://blog.ionicframework.com/the-end-of-framework-churn/