marko-js-archive / marko-widgets

[LEGACY] Module to support binding of behavior to rendered UI components rendered on the server or client
http://v3.markojs.com/docs/marko-widgets/
MIT License
142 stars 40 forks source link

Proposal: Marko Widgets with ES6 and inline Marko templates #152

Open patrick-steele-idem opened 7 years ago

patrick-steele-idem commented 7 years ago

We are exploring how we can improve Marko Widgets by leveraging the ES6 syntax and here are the initial thoughts:

import { Component } from 'marko-widgets';

export default class extends Component {

    render(state, input) {
        var fullName = state.firstName + ' ' + state.lastName;

        return marko`<div> <!-- w-bind is implicit -->
            <app-hello w-id="hello" w-onclick='handleButtonClick' name=fullName />
        </div>`;
    }

    constructor() {
        // This is called on the server and in the browser, but you
        // probably don't want to do anything here

        // *Don't* call super: super();

        // this.el === undefined
    }

    onInput(input) {
        this.state = {
            firstName: this.state.firstName || input.firstName,
            lastName: input.lastName
        };

        // Any additional properties will be serialized to the browser if rendered on the server
        this.foo = { hello:'world' };
    }

    onMount() { // This is a new lifecycle event method
        // this.el is available here, but not in the constructor
        var el = this.el;

        this.helloWidget = this.getWidget('hello');
    }

    handleButtonClick(event, el) {
        this.doSomething(); // This is okay
    }

    doSomething() {

    }
};

Feedback is appreciated

abiyasa commented 7 years ago

Looks great! 👍 👍 👍

I really like the compact code. Combined with inline template, we could have a Marko Widget component in just 1 file. Well, we still have marko-tag.json file, but maybe we could combine that into the class using 'static' property.

export default class CustomComponent extends Component {

    render(state, input) {
        var fullName = state.firstName + '
...
}

CustomComponent.tag = {
    attributes: {
        model: 'expression'
    }
};

What do you think? Some other questions:

patrick-steele-idem commented 7 years ago

Well, we still have marko-tag.json file, but maybe we could combine that into the class using 'static' property.

For the most part, marko-tag.json is optional, but it is used compile-time validation and it is also used by third-party tools. I think it is best to keep the marko-tag.json in a separate JSON file to avoid the overhead of having to parse the entire JS file just to find the tag def since that would slow down autocomplete and compilation.

Is the new onMount() replacing the old init()? That's cool & reminds me of React's componentDidMount

Yes, onMount() would replace init(). Having similarity with other view libraries is a good thing and to deviate where it makes sense for performance reasons and usability (e.g. cleaner Marko syntax).

How can we implement split renderer & widget in ES6?

Good question. We do plan on keeping complete backwards compatibility so you could always use the old syntax, but it probably makes sense to offer extends Renderer and extends Widget as an alternative to extends Component.

Thanks for the feedback @abiyasa!

abiyasa commented 7 years ago

Thanks for the reply. I'm not aware of that use case for marko-tag.json. Make more sense to keep it as separated file then.

We do plan on keeping complete backwards compatibility so you could always use the old syntax, but it probably makes sense to offer extends Renderer and extends Widget as an alternative to extends Component.

Extending class from Renderer or Widget classes sounds good!

basickarl commented 7 years ago

Sneaking in here and saying that the syntax looks good. As @abiyasa said would be nice to somehow avoid writing import { Component } from 'marko-widgets';, on the other hand it would be needed to define extends Renderer and extends Widget (which is a good idea).

I was looking at reactjs syntax and they skip the export default, any reason for keeping it here (thinking as you said earlier, would be good to get the syntax closer to each other)?

Again looking at reactjs they state React components can have state by setting this.state in the constructor. I'm guessing in your example that is the same as onInput? I'm guessing it's not in the constructor as onInput can be invoked several times (change state at runtime)?

Woudl also be nice to get rid of the exports.render = require('./renderer').render; in the index.js when splitting up the Component into the Wdiget and Renderer'.

Apart from that looks awesome, hows it going? :P

patrick-steele-idem commented 7 years ago

Thanks for the input, everyone. @mlrawlings had started the work on this, but after some discussion we realized we can simplify things even more. We still want to include support for onMount and onInput, but with the new proposal we see no benefit from supporting ES6. I will write up a new proposal based on our discussions for feedback and link here once done. I'm confident that everyone will like the simplifications with the new proposal :)

patrick-steele-idem commented 7 years ago

Please see: Issue #167 - Proposal: Template as entry point for UI components