Matt-Esch/virtual-dom

Method of applying properties does not fire property changed setters/getters

brokenalarms opened this issue · 0 comments

Hi

We are using an extended implementation of web components, supporting passing of objects via getters and setters as well as the native web components' attributeChanged callback.

For example, a component like <my-d3-graph /> will re-render or update when its 'data' property is changed, via a setter on 'data' that calls the component's render function:

        const webComponentName = 'my-d3-graph';
        const props = {
            attached() {
           // extension of native 'attachedCallback'
                d3.select(this).append('svg');
            },
            detached() {
           // extension of native 'detachedCallback'
                d3.select(this).select('svg').remove();
            },
            data: {
                // extension for object properties to call update functions
               // just like the 'attributeChangedCallback' but for objects instead of strings
                propertyChanged(newData) {
                    render.call(this);
                },
            },
        };
        ourWebComponentExtensionLibrary.register(webComponentName, props);

There are two resulting issues from this with your method of assigning props in apply-properties.js:

  1. On first diff, the empty object is created with the following code, before the required values are moved across:

        if (!isObject(node[propName])) {
            node[propName] = {}
        }

    This fires the propertyChanged callback and re-renders the component before there is any data available to draw. We could solve this one by putting the propertyChanged callback back onto the queue (setTimeout 0) so that the update logic would not execute synchronously before the props were available, however;

  2. On subsequent diffs, the properties are just copied across since the root property already exists; thus the web component never updates again.

Both of these issues would be fixed by changing that code to the following:

        var newObj = Object.assign({}, node);
        for (var k in propValue) {
            var value = propValue[k]
            newObj[propName] = {k: (value === undefined) ? replacer : value}
        }
       // assignment now fires propertyChanged callback once new property is actually updated.
        node[propName] = newObj;

This seems to be a use case unthought of in the implementation of vdom, but having getters and setters on the root props of DOM nodes seems like a reasonable implementation, especially for web components. In this situation, the virtual DOM is used for its ability to express the DOM within JS and to update both attributes and properties as required, but not necessarily triggering a DOM layout change.

This is obviously not my specialty so I may not have fully considered the implications this has for duplicating DOM layout updates or the like. But at least on the props level, because you are only replacing the new root properties of the node (and not recursively deep merging), altering the order of assignment in the code above would fix our issues.

If this is an acceptable solution for you or there are no other factors I haven't considered, I could do a PR for this (using the lodash-imported version of assign/extend)?

Thanks!