google / incremental-dom

An in-place DOM diffing library
http://google.github.io/incremental-dom/
Apache License 2.0
3.54k stars 180 forks source link

patchOuter on detached node #318

Closed atomictag closed 7 years ago

atomictag commented 7 years ago

I am not sure why this works:

var el = document.createElement('div'), counter = 0;

var fn = function() {
    IncrementalDOM.elementOpen('div', null); // <--- no `key` here
        IncrementalDOM.text('counter: ' + counter++);
    IncrementalDOM.elementClose();
};

var render = function() {
    IncrementalDOM.patchOuter(el, fn);
    setTimeout(render, 1000);
}

render();
someDiv.appendChild(el);

but as soon as the patch function looks like this

var fn = function() {
    IncrementalDOM.elementOpen('div', 'SOME_KEY'); // <--- `key` is set here
        IncrementalDOM.text('counter: ' + counter++);
    IncrementalDOM.elementClose();
};

I get Cannot read property '__incrementalDOMData' of null when elementOpen is executed the first time because the parent does not exist - and alignWithDOM calls getData(currentParent) which throws if currentParent is null. I understand that keys are managed within the parent, but what if there is none?

I am working on a Handlebars "backend" - which generates keys automatically if only static properties are used - but this test is breaking and I am not sure I am missing some important points about keys etc. here. Thanks

sparhami commented 7 years ago

I think one issue this brings up is what is the behavior when you append a child with a key to a container. I'm not sure there is a good way for that container to know about the new keyed item for the next patch.

Incremental DOM will have a new API soon, where you can use a typeId instead of a generated key to allow safe application of statics.

I think that keys really only make sense in the context of a parent, so it might be best to throw an Error if you try to do patchOuter of a detached element with a key.

atomictag commented 7 years ago

Thanks. Looking forward to the new API. For the time being I resorted to a small hack that ignores the key of the first elementOpen/elementOpenStart within a patchOuter if the element has no parent. Seems to work ok with container-less views

jridgewell commented 7 years ago

Does it work if you change the tagname? I'd rather always throw when calling patchOuter on a detached node, like outerHTML does:

var d = document.createElement('div');
d.outerHTML = '<span></span>'; // => Error!
atomictag commented 7 years ago

Nope it does not work if the tag is changed. I agree the obvious behaviour of patchOuter should be to throw if the node is detached. However some frameworks allow container-less views where the root node is provided by the template itself - so patchOuter seems the only option here (as long as the tag does not change of course).

sparhami commented 7 years ago

However some frameworks allow container-less views where the root node is provided by the template itself - so patchOuter seems the only option here (as long as the tag does not change of course).

You could do patchInner on a DocumentFragment (created by document.createDocumentFragment) instead. The key would still be used of equality checking, but may not be used for reuse if the order of things changes.

atomictag commented 7 years ago

Yes, that seems the ticket:

         var rendered;
         if(!this.el.parentNode) {
             // detach render: call patch on a fragment
             var frag = document.createDocumentFragment();
             rendered = IncrementalDOM.patch(frag, template, context)
             rendered = frag.firstChild;
         } else {
             // call patchOuter as the root node
             rendered = IncrementalDOM.patchOuter(this.el, template, context);
         }

         if(rendered !== this.el) {
              // if the root element has changed we need to set it again in the view
              // ...
         }

no issues with ids here and this works also if the tagName changes as noted by @jridgewel many thanks!