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

Updating properties.classes not being reflected in the DOM #49

Closed
kitsonk opened this issue Apr 28, 2016 · 6 comments
Closed

Updating properties.classes not being reflected in the DOM #49

kitsonk opened this issue Apr 28, 2016 · 6 comments

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Apr 28, 2016

Given the following:

import { createProjector, h, VNode } from 'maquette/maquette';

const node = h('dojo-panel-tabbed', {
    id: 'tabbed-panel'
}, [
    h('ul', {}, [
        h('li', { classes: { active: false }, key: {} }, [ h('div.tab-label', [ 'tab 1' ]), h('div.tab-close', [ 'X' ]) ]),
        h('li', { classes: { active: true }, key: {} }, [ h('div.tab-label', [ 'tab 1' ]), h('div.tab-close', [ 'X' ]) ]),
        h('li', { classes: { active: false }, key: {} }, [ h('div.tab-label', [ 'tab 1' ]), h('div.tab-close', [ 'X' ]) ]),
        h('li', { classes: { active: false }, key: {} }, [ h('div.tab-label', [ 'tab 1' ]), h('div.tab-close', [ 'X' ]) ])
    ])
]);

function render(): VNode {
    return node;
}

const projector = createProjector({});

projector.append(document.body, render);

const next = document.getElementById('next');

next.addEventListener('click', (event) => {
    console.log('before', node.children[0].children[1].properties.classes, node.children[0].children[2].properties.classes);
    node.children[0].children[1].properties.classes['active'] = false;
    node.children[0].children[2].properties.classes['active'] = true;
    console.log('after', node.children[0].children[1].properties.classes, node.children[0].children[2].properties.classes);
    projector.scheduleRender();
});

Maquette does not appear to notice changes to changes to already rendered VNodes. The classes do not change if a VNode is updated. I maybe missing something, but is it not possible to reuse VNodes?

To explain a bit, where I ran into an issue trying to create a tabbed component, where the VNodes for the tab bar were always regenerated, but the tab's contents would only be calculated for the tab coming into view, but then updating the classes of any tab contents already rendered. That way, if the tab were to come back into view, but its content had not changed, it would be as simple as adding a class back.

I have noticed though if I dropped a VNode out of one render and then return it later, the VNode will be full parsed.

I even tried to "reuse" just VNodeProperties, but that also appeared to exhibit the same behaviour, that if it was present from one render to the next, changes to its properties were not deeply noticed.

I can get it to work, only if I do something like this:

import { createProjector, h, VNode } from 'maquette/maquette';

let clicked = false;

function render(): VNode {
    return h('dojo-panel-tabbed', {
        id: 'tabbed-panel'
    }, [
        h('ul', {}, [
            h('li', { classes: { active: false }, key: {} }, [ h('div.tab-label', [ 'tab 1' ]), h('div.tab-close', [ 'X' ]) ]),
            h('li', { classes: { active: !clicked }, key: {} }, [ h('div.tab-label', [ 'tab 1' ]), h('div.tab-close', [ 'X' ]) ]),
            h('li', { classes: { active: clicked }, key: {} }, [ h('div.tab-label', [ 'tab 1' ]), h('div.tab-close', [ 'X' ]) ]),
            h('li', { classes: { active: false }, key: {} }, [ h('div.tab-label', [ 'tab 1' ]), h('div.tab-close', [ 'X' ]) ])
        ])
    ]);
}

const projector = createProjector({});

projector.append(document.body, render);

const next = document.getElementById('next');

next.addEventListener('click', (event) => {
    clicked = true;
    projector.scheduleRender();
});

In my opinion, this is rather surprising behaviour and means that it is difficult to not fully recalculate the whole of the virtual DOM on every render, which doesn't properly represent the application state, as from render to render, only small changes are made to the VDOM.

@johan-gorter
Copy link
Contributor

It is true that maquette assumes that VNodes are never modified. This is documented here: http://maquettejs.org/docs/typedoc/interfaces/_maquette_.vnode.html.

If maquette encounters the same VNode instance, it assumes you did not make modifications to it and it therefore skips diffing the VNode. Diffing the VDOM is generally slower than calculating the VDOM, so if you really want to optimize for performance, you should try to skip diffing the VDOM as well.

This is where the createCache function comes in. If you rewrite your code as follows, both rerendering AND the diffing of the tabbedpanel is skipped if nothing was changed. Also, the logic of adding/removing the active class is only specified once.

import { createProjector, h, VNode } from 'maquette/maquette';

let clicked = false;

let renderCache = createCache<VNode>();

let render = (): VNode => {
  return renderCache.result([clicked], () => h('dojo-panel-tabbed', {
    id: 'tabbed-panel'
  }, [
      h('ul', {}, [
        h('li', { classes: { active: false }, key: {} }, [h('div.tab-label', ['tab 1']), h('div.tab-close', ['X'])]),
        h('li', { classes: { active: !clicked }, key: {} }, [h('div.tab-label', ['tab 1']), h('div.tab-close', ['X'])]),
        h('li', { classes: { active: clicked }, key: {} }, [h('div.tab-label', ['tab 1']), h('div.tab-close', ['X'])]),
        h('li', { classes: { active: false }, key: {} }, [h('div.tab-label', ['tab 1']), h('div.tab-close', ['X'])])
      ])
    ]));
};

const projector = createProjector({});

projector.append(document.body, render);

const next = document.getElementById('next');

next.addEventListener('click', (event) => {
  clicked = true;
  projector.scheduleRender();
});

I hope you like this approach, and hopefully you understand the design decision of disallowing VNode modifications.

Any suggestions how we could make this clearer in the documentation would be beneficial.

We will probably be marking the VNode properties as readonly when Typescript2 comes out, which may prevent some future mistakes.

@kitsonk
Copy link
Contributor Author

kitsonk commented Apr 28, 2016

Sorry for transforming this into a more "support" issue, but trying to get my mind around your suggested approach, but it seems I may have been over thinking a solution when you have provided something that actually solves the same problem.

Essentially the components have their own internal cache on their render functions which maintain their own invalidated/dirty state. If they have been rendered, but are not invalidated they simply return the cached render, which would include any children components rendered VNodes. Only when the component is invalidated it invalidates it parent, up to the point where it reaches the projector which then schedules a render.

I will try to think about it, but if you have any suggestions about how to leverage the CalculationCache in these complex nested child/parent render functions, that would be very much welcomed.

@johan-gorter
Copy link
Contributor

First of all, I do not see the point in making a complex component system where components invalidate their parent. The component pattern that is explained on the maquette website works very well, even when pages are getting complex. Just re-rendering every component is usually not a problem at all, even less powerful devices can achieve 60fps on large complex pages.

Parent-child relations can simply be formed by calling the child's renderMaquette from the parent component's renderMaquette, that's all.

Having said that, you can still use the CalculationCache, because it can be invalidated using its invalidate method, which may just be what you were looking for. Just pass in an empty array as the first parameter to its result function.

@kitsonk
Copy link
Contributor Author

kitsonk commented Apr 28, 2016

I understand what you are saying and I appreciate your thoughts and help.

Just re-rendering every component is usually not a problem at all, even less powerful devices can achieve 60fps on large complex pages.

This assumes that the biggest load in calculating the VDOM is actually calculating what the nodes are and their properties. In Dojo 1 (and we expect in Dojo 2) that many use cases will be complex applications with many hundred components running fairly large single page apps, where only portions are visible at any given time. In these cases the visible DOM is only the tip of the iceberg, where being rendered can trigger a lazy load of back-end data or a scroll event can require complex DOM areas to be calculated, there is a need to be able to have components aware of the validity of their state.

@johan-gorter
Copy link
Contributor

It is certainly an interesting dillemma, to what limit can maquette (or any other virtual DOM library) be able to deliver 60fps, and at what point will rendering an diffing take up more than 16ms. There is certainly a limit, I know.

You mention 'where only portions are visible at any given time'. This means that the invisible portions will not be in the VDOM and will thus not affect performance (I would certainly discourage using display: none).

I think that the CalculationCache can help you out, or you can even make your own cache if you like. I think maquettes assumption that VNodes should not be modified actually helps you to scale here, because it also bypasses diffing that part of the VNode tree.

@kitsonk
Copy link
Contributor Author

kitsonk commented May 2, 2016

You mention 'where only portions are visible at any given time'. This means that the invisible portions will not be in the VDOM and will thus not affect performance (I would certainly discourage using display: none).

Yes, but often, figuring out if the VDOM should be in the DOM or can take calculations that are outside of the scope of the VDOM, therefore my attempt to "cache" VDOM.

I also agree about the display: none with one exception. For example, a tabbed container component. It may logically have 2 tabs, of which only one is visible at any given time. Obviously, initially, it make no sense to calculate the VDOM for the non-visible remaining tabs. When switching to tab 2, it also doesn't make sense to recalculate tab 1, but since it is already in the flow of the DOM, it does potentially make sense to just make that not visible via a class change and add the new DOM for tab 2. If tab 1 becomes visible again, its VDOM would be recalculated, but likely would result in minimal if actual DOM mutations. While maquette is efficient at swapping the DOM for tab 1 and tab 2, if that DOM is complex, every tab switch can result in a lot of creation of document fragments with complex DOM trees for which most likely there were minimal changes to the DOM/VDOM while the tab was not visible. That specifically was the use case I was working on.

I agree the overall design is the the "right" thing to do, as far as the CalculationCache. I had essentially come to a similar solution for a problem only to find out that Maquette already solved it!

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

No branches or pull requests

2 participants