WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.07k stars 112 forks source link

hyper.Component discards whole DOM Node #206

Closed jaschaio closed 6 years ago

jaschaio commented 6 years ago

Coming from reacts virtual dom I am not sure if this is right or wrong. But it seems that hyper.Component discards the whole DOM instead of only updating the parts that are different.

It's best shown in this gif. The first part is react and it only switches out the classes. The second part is hyperHTML and it switches out the complete DOM Node.

hyperhtml

This is the code I am using:


class App extends hyper.Component {

    get defaultState() {
        return {
            activePanel: null
        };
    }

    constructor() {
        super();
        this.navItems = [
            'Dashboard',
            'Posts',
            'Pages',
            'Products',
            'Design',
            'Settings',
            'Collapse'
        ];
    }

    onclick(event) {
        event.preventDefault();

        if ( ! event.target.closest('a.nav-link') ) {
            this.setState({ activePanel: null });
            return;
        }

        let activePanel = event.target.closest('a.nav-link').getAttribute('data-panel');
        this.setState({ activePanel: activePanel });

    }

    renderNavItem( currentValue, index, array ) {

        let active = ( this.state.activePanel === currentValue ) ? 'active' : 'inactive';

        return  hyper.wire()`
            <li class="${ active }">
                <a onclick=${ this } class="${ 'nav-link ' + currentValue.toLowerCase() }" data-panel="${ currentValue }">
                    <span>${ this.navItems[currentValue] }</span>
                </a>
            </li>
        `;
    }

    render() {
        return this.html`
            <div class="App">
                <div>
                    <div class="nav">
                        <div class="logo">
                            <a onclick=${ this }></a>
                        </div>
                        <ul>
                            ${ this.navItems.map( ( currentValue, index, array ) => this.renderNavItem( currentValue, index, array ) ) }
                        </ul>
                    </div>
                    <div class="panel"></div>
                </div>
                <div id="editor"></div>
            </div>
        `;
    }
}

hyper(document.body)`${new App}`;
joshgillies commented 6 years ago

The problem here is related to the use of an unbound wire in your renderNavItem function.

I've inlined an updated version of your renderNavItem with the following piece of documentation in mind: https://viperhtml.js.org/hyperhtml/documentation/#essentials-3

    renderNavItem(currentValue, index, array ) {

        let active = ( this.state.activePanel === currentValue ) ? 'active' : 'inactive';

        // Binds our list item fragment to the App instance
        // With an unique identifier of `:nav-item-${index}`
        return  hyper.wire(this, `:nav-item-${index}`)`
            <li class="${ active }">
                <a onclick=${ this } class="${ 'nav-link ' + currentValue.toLowerCase() }" data-panel="${ currentValue }">
                    <span>${ this.navItems[currentValue] }</span>
                </a>
            </li>
        `;
    }

I'm on my phone at the moment so may have mistyped something but hopefully this above helps. 👍

jaschaio commented 6 years ago

Thanks a lot @joshgillies, you are right it does work now:

https://codepen.io/jaschaio/full/xYedOy

Unfortunately it stops working as soon as I try to nest several hyper.Components and pass state from one to another.

Here is the pen: https://codepen.io/jaschaio/full/ZrZKKx/

There is a method called "changeActivePanel" which is passed down from the parent component and binded to the parent components state. This method is responsible for chaging the state in the parent component from within the nested one.

But as soon as I do that the whole DOM node gets discarded instead of just the changes. (So the same as before).

class Nav extends hyperHTML.Component {
    constructor() {
        super();
        this.navItems = [
            'Dashboard',
            'Posts',
            'Pages',
            'Products',
            'Design',
            'Settings'
        ];
    }

    onclick(event) {
        event.preventDefault();

        if ( ! event.target.closest('a.nav-link') ) {
            this.changeActivePanel( null );
            return;
        }

        let activePanel = event.target.closest('a.nav-link').getAttribute('data-panel');
        this.changeActivePanel( activePanel );

    }

  renderNavItem(currentValue, index, array ) {

        let active = ( this.activePanel === currentValue ) ? 'active' : 'inactive';

        // Binds our list item fragment to the App instance
        // With an unique identifier of `:nav-item-${index}`
        return  hyperHTML.wire(this, ':nav-item-' + index)`
            <li class="${ active }">
                <a onclick=${ this } class="${ 'nav-link ' + currentValue.toLowerCase() }" data-panel="${ currentValue }">
                    <span>${ this.navItems[index] }</span>
                </a>
            </li>
        `;
    }

    render() {
      return this.html`
      <ul>
         ${ this.navItems.map( ( currentValue, index, array ) => this.renderNavItem( currentValue, index, array ) ) }
      </ul>
      `
    }

}

class App extends hyperHTML.Component {

    get defaultState() {
        return {
            activePanel: null,
        };
    }

    constructor() {
        super();
        this.changeActivePanel = this.changeActivePanel.bind( this );
    }

    changeActivePanel( activePanel ) {
        this.setState({
            activePanel: activePanel
        });
    }

    render() {
        var nav = new Nav;
        nav.activePanel = this.state.activePanel;
        nav.changeActivePanel = this.changeActivePanel;
        return this.html`
            <div class="App">
                <div>
                    <div class="nav">
                        <div class="logo">
                            <a onclick=${ this }></a>
                        </div>
                        ${ nav }
                    </div>
                    <div class="panel"></div>
                </div>
                <div id="editor"></div>
            </div>
        `;
    }
}

hyperHTML(document.body)`${new App}`;

Again: I am coming from react, maybe I am just not completely grasping the concepts of hyperHTML yet. I found that the documentation only has fairly simple examples for now which makes it hard to understand how nesting components and passing around state and props works with hyperHTML.

joshgillies commented 6 years ago

Yeah there some drastic differences in the way component composition is managed with hyperHTML when compared to React. I'm on the move still, though check out this PR https://github.com/WebReflection/hyperHTML/pull/202 wich features discussion around this exact subject with an addition to a new core API to help with this. Though I'm not sure if the documentation has been updated yet to reflect these changes.

jaschaio commented 6 years ago

Got it working with the new declarative component approach from #202.

There are a couple of things I am not sure about or don't quite understand yet, so would be great to see an updated documentation.

For example:

Anyway, here is the updated and working code

class Nav extends hyperHTML.Component {
    constructor( props ) {
        super();
        this.navItems = [
            'Dashboard',
            'Posts',
            'Pages',
            'Products',
            'Design',
            'Settings'
        ];
        this.update( props );
    }

    update( props ) {
        this.props = props;
        this.setState( props, false );
        return this.render();
    }

    onclick(event) {
        event.preventDefault();

        if ( ! event.target.closest('a.nav-link') ) {
            this.changeActivePanel( null );
            return;
        }

        let activePanel = event.target.closest('a.nav-link').getAttribute('data-panel');
        this.changeActivePanel( activePanel );

    }

  renderNavItem(currentValue, index, array ) {

        let active = ( this.state.activePanel === currentValue ) ? 'active' : 'inactive';

        // Binds our list item fragment to the App instance
        // With an unique identifier of `:nav-item-${index}`
        return  hyperHTML.wire(this, ':nav-item-' + index)`
            <li class="${ active }">
                <a onclick=${ this } class="${ 'nav-link ' + currentValue.toLowerCase() }" data-panel="${ currentValue }">
                    <span>${ this.navItems[index] }</span>
                </a>
            </li>
        `;
    }

    render() {
      return hyperHTML.wire(this, ':nav')`
      <ul>
         ${ this.navItems.map( ( currentValue, index, array ) => this.renderNavItem( currentValue, index, array ) ) }
      </ul>
      `
    }

}

class App extends hyperHTML.Component {

    constructor( props = { activePanel: null } ) {
        super();
        this.changeActivePanel = this.changeActivePanel.bind( this );

        this.Nav = new Nav( props );
        this.Nav.changeActivePanel = this.changeActivePanel;

        this.update( props );
    }

    update( props ) {
        this.props = props;
        this.setState( props, false );
        return this.render();
    }

    changeActivePanel( activePanel ) {
        this.update({
            activePanel: activePanel
        });
    }

    render() {
        this.Nav.update( this.state );
        return hyperHTML.wire(this, ':app')`
            <div class="App">
                <div>
                    <div class="nav">
                        <div class="logo">
                            <a onclick=${ this }></a>
                        </div>
                        ${ this.Nav }
                    </div>
                    <div class="panel"></div>
                </div>
                <div id="editor"></div>
            </div>
        `;
    }
}

hyperHTML(document.body)`${
    App.for(document).update()
}`;
WebReflection commented 6 years ago

There are a couple of things I am not sure about or don't quite understand yet, so would be great to see an updated documentation.

my apologies I've been doing other things and didn't update documentation around the new components syntax, but the concept is that you don't need to store references in the constructor unless you want to do that for other reasons.

Example, instead of the following:

this.Nav.update( this.state );
// ... later on ...
${ this.Nav }

You'd do something like:

${ Nav.for(this).update(this.state) }

but in above specific examples there is a shared changeActivePanel reference I'm not sure I fully understand.

Shared outer/global states shouldn't really be a matter of sub components, rather handled in the outer/global one and through bubbling events, IMO, but I don't have a proper whole picture to suggest a better approach.

Other answers coming.


What's the second argument of the setState method for

when you update the state the component automatically renders itself to reflect the new state. I could've done fancy with timers or requestAnimationFrame and cancel those each time or I could've gone synchronously, which has been enough for the last year.

However, there are cases where you might want to set state, maybe multiple times, without rendering per each change (think about delegated states updates) so in that case you can drop the default render call passing false as second argument.

This is documented via the TypeScript definition but not via official documentation, sorry.

If you need/want to force the render each time though, since when the component is always the same its render won't be called (same goes for same nodes) you can simply use setState.

TL:DR instead of this:

    update( props ) {
        this.props = props;
        this.setState( props, false );
        return this.render();
    }

you can write this:

    update( props ) {
        return this.setState( props );
    }

Latest setState version returns the component itself and it invokes render by default. I'd also argue if your state is fully represented by external props you don't need to hold both props and state per each component, just use the state.


Where should child components be defined? (Currently I do it in the constructor)

It depends. If you need a reference to sub components you want to attach that in the constructor. If you don't care, like in this example, you can use the shortcut to weakly reference a component to one known object (which inside another component could be the this reference itself).

If you want to keep components portable it's convenient to have empty constructors and, eventually, or if needed, an update(...) method that does either an initial setup or returns, the component or its rendered content.

Keeping the constructor clean gives you the ability to render via new Comp().update() or Comp.for(ref).update() without needing to pass any constructor argument.


When should I call the .update() method on child components? (Within the render or in the update method of the parent component itself).

The concept is that since hyperHTML might call .render() internally at any time and without warning (example when you change/swap child components) it's a good practice to keep .render() a clean function that can do many things but receive no arguments and return this.html or this.svg with the content.

However, if you want to pass properties down, then you need a mechanism, like an update(props), that could provide such functionality.

These are all convenient ways to deal with components, but none of them is really mandatory, these are just suggested patterns anyone can change. abuse, improve, as they need.

If you want a more opinionated way to have sub components, then Custom Elements would be my suggestion so you keep the layout HTML declarative and you follow CE updates/upgrades mechanisms instead, including attribute changes.

I hope this answered your questions. if you have more, please ask. Thanks.

WebReflection commented 6 years ago

waiting 'till tomorrow evening then I'll close since there's no action for me to take.

jaschaio commented 6 years ago

Thanks for your reply this actually already helped a lot to rework and simplify some parts of my components. Looking forward to an updated documentation and if I encounter any more related questions I will just follow up in this issue.