Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
blikblum opened this issue Dec 22, 2017 · 16 comments
Open

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

blikblum opened this issue Dec 22, 2017 · 16 comments

Comments

@blikblum
Copy link
Collaborator

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

  • Align with current web technology
  • Remove hard dependency on Backbone
  • Less/simplified code
  • First class interoperability with other frameworks

Cons

  • Big breaking change
  • No destroy lifecycle for view
  • Not possible to create a CollectionView with ul/li tags (the custom element name must be separated by a dash)
@paulfalgout
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Contributor

JSteunou commented Dec 30, 2017

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
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Contributor

JSteunou commented Jan 2, 2018

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
Copy link
Member

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
Copy link
Collaborator Author

blikblum commented Jan 2, 2018

In any case, the changes should be made post v4

@paulfalgout
Copy link
Member

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
Copy link
Collaborator Author

blikblum commented Jan 2, 2018

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

I think so

@JSteunou
Copy link
Contributor

JSteunou commented Jan 3, 2018

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
Copy link
Collaborator Author

blikblum commented Jan 5, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants