search

crysalead-js / dom-layer

Virtual DOM implementation.
MIT License
30 stars 1 forks source link

Fix patch operations to assign parent #73

Closed brandonpayton closed 8 years ago

brandonpayton commented 8 years ago

Here is the PR you requested.

reproduces #71

jails commented 8 years ago

Can you push the fix at the same time ? You just need to replace calls by render(container, parent) instead of render(parent)

brandonpayton commented 8 years ago

I was thinking you'd just want to call render(undefined, parent) to avoid the additional appendChild call before immediately calling insertBefore, but neither change causes the tests to pass. I'm not yet sure what is going on.

brandonpayton commented 8 years ago

I was thinking it was because Tag#patch and Text#patch do not assign to.parent = this.parent, but making that update didn't lead to passing tests either.

brandonpayton commented 8 years ago

I found the remaining issue. The Tag#patch did need to assign its parent as mentioned above, but it also needed to pass a parent argument to the tree/update function. I've updated this PR with fixes and went with the render(undefined, parent) approach mentioned above. Let me know if you'd like me to change it back to render(container, parent).

brandonpayton commented 8 years ago

btw, there is probably a better place for the tests to go. I just placed them where I did for expedient repro. Setting parent in patch methods and ensuring parent is set for new nodes are separate concerns.

jails commented 8 years ago

Good catch, looks fine ! Btw why did you use render(undefined, parent) instead of render(container, parent). It seems not correct.

brandonpayton commented 8 years ago

I used render(undefined, parent) because otherwise Tag#render will append them to the container with appendChild immediately before tree/patch moves them via insertBefore. It seemed better to allow Tag#render to continue adding to the document fragment for now, but I don't have numbers to prove it is better. If you prefer render(container, parent), let me know, and I'll switch to that.

jails commented 8 years ago

If the virtual tree isn't mounted the container variable will be undefined. And if the virtual tree is mounted we should patch using the real DOM container imo.

brandonpayton commented 8 years ago

Hi @jails, sorry for the delay. Do I understand correctly that you are OK with the fact that rendering new nodes into a mounted tree will first add them using appendChild and immediately follow with insertBefore? I am OK with providing that so we can close this out, but I am concerned it will impact performance.

jails commented 8 years ago

Fixed in https://github.com/crysalead-js/dom-layer/commit/e9a62805646cbec0d17f8b9dd223c1f3efa2046d