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

Node level lifecycle hooks not always firing #165

Closed
tropperstyle opened this issue Jul 11, 2017 · 15 comments
Closed

Node level lifecycle hooks not always firing #165

tropperstyle opened this issue Jul 11, 2017 · 15 comments
Labels

Comments

@tropperstyle
Copy link

Is there any guarantee that a node's willInsert hook will be called? I have a scenario where an element ends up in the dom without firing any lifecycle hooks. On a later redraw, willRecycle is finally the first hook to fire.

el('div.some-class', {
    _hooks: {
        willInsert: function(node) {
            console.log("willInsert", node);
        },
        willRecycle: function(oldNode, newNode) {
            console.log("willRecycle", oldNode, newNode);
        },
        willReinsert: function(node) {
            console.log("willReinsert", node);
        },
        willRemove: function(node) {
            console.log("willRemove", node);
        }
    }
})

I also have another scenario that sometimes barfs because node.dropdown === null in didRemove:

el('div.phone-input', {
    _hooks: {
        didInsert(node) {
            node.dropdown = new CountryDropdown();
        },
        willRecycle(oldNode, newNode) {
            newNode.dropdown = oldNode.dropdown;
        },
        didRemove(node) {
            node.dropdown.destroy();
        }
    }
})

I'm guessing these mismatches are due to the dom element pooling. I tried adding _key without luck. Maybe I am missing something. Is a sub-view required to encapsulate this type of life-cycle?

@leeoniya
Copy link
Member

I'm guessing these mismatches are due to the dom element pooling

yes. besides will/didRecycle, vnodes hooks are fired by the dom reconciler in conjunction with dom insertion/reorder/removal events. the old vnode's willRecycle should fire when its dom element is adopted by domvm to represent a new vnode and the element is patched to match it. in an unkeyed scenario this effectively means that you need to have the hooks defined on both the old and the new vnodes.

I tried adding _key without luck...Is a sub-view required to encapsulate this type of life-cycle?

keying the nodes should work to prevent dom recycling [1], however, it doesnt prevent new vnode creation. everything you want on the vnode must be (re)defined on the new vnode during each render cycle. if you need to store anything on the vnode, you should use {_data:...} in attrs, which becomes accessible as vnode.data. the only state that automatically carries over from the old vnode is the dom element. vnodes should be considered immutable and treated as discardable on each redraw() of any ancestor vm.

i'm gonna close this, but let me know if further clarification is needed. i'll add this to the WIP docs todo.

[1] http://leeoniya.github.io/domvm/demos/playground/#lifecycle-hooks

@leeoniya leeoniya mentioned this issue Jul 11, 2017
@leeoniya
Copy link
Member

leeoniya commented Jul 11, 2017

oops, you're right. i need to test your issues more carefully since i always seem to explain things you already understand. there's a didRemove bug here [1]. willRemove works though.

EDIT: interestingly, didRemove does fire if willRemove is also defined: https://jsfiddle.net/m27337ud/

const el = domvm.defineElement;

let show = true;

const _hooks = {
  willRemove: (n) => console.log("willRemove"),
  didRemove: (n) => console.log("didRemove"),
};

const View = {
  render: (vm, data) =>
    el("div", [
      show ? el('div', {_hooks: _hooks}) : null
    ])
};

var vm = domvm.createView(View).mount(document.body);

setTimeout(() => {
  show = false;
  vm.redraw();
}, 1000);

[1] https://jsfiddle.net/mayau0wk/

@leeoniya leeoniya reopened this Jul 11, 2017
@leeoniya leeoniya added the bug label Jul 11, 2017
@leeoniya
Copy link
Member

this should be fixed. thanks for the report!

@tropperstyle
Copy link
Author

I think you may have fixed a separate issue. In my case, none of the will* hooks ever fire on the initial render. On a subsequent redraw willRecycle is the first hook called.

Does that sound normal?

@leeoniya
Copy link
Member

leeoniya commented Jul 12, 2017

no, that sounds abnormal. most of the vnode hooks fire here [1] just before actual dom insertion/re-insertion. i dont see how they would not fire unless your view is not actually mounted.

are you able to repro in a fiddle?

[1] https://github.com/leeoniya/domvm/blob/3.x-dev/src/view/dom.js#L102

@leeoniya
Copy link
Member

leeoniya commented Jul 12, 2017

there is a difference between whether key changes of vm root nodes cause dom node removal/recreation.

EDIT: now fixed by 8990398 (below examples should all work the same after jsfiddle's cache expires)

plain template dom elements will get removed and recreated:
https://jsfiddle.net/5zb6k10s/

vm root dom elements will always be recycled:
https://jsfiddle.net/5zb6k10s/1/
https://jsfiddle.net/5zb6k10s/2/

but if the key is defined at the view level, then it works the same as the first case:
https://jsfiddle.net/5zb6k10s/3/

this is an inconsitency that i'll open another bug for. i should be able to make all cases work like the first.

@tropperstyle
Copy link
Author

https://jsfiddle.net/kmcrqfw7/

In both Safari and Chrome willRecycle and didRecycle are the only hooks logged. Also interesting to note that they only fire every other redraw.

If you remove the array around main content, then the only hooks logged are willInsert and didInsert

This seems like a pretty basic tree and makes me think #164 is real.

@leeoniya
Copy link
Member

leeoniya commented Jul 12, 2017

the way your example behaves is by design.

you're only defining hooks on the Alt Content vnode, so they will only fire if you originally mount with that vnode (will/didInsert) or when you redraw away from that vnode (will/didRecycle). the former never happens because you mount with Main Content and the latter happens every other redraw call because you're recycling the dom element represented by that vnode at that time every other call.

your original, a bit easier to read: https://jsfiddle.net/kmcrqfw7/1/
with hooks on both nodes: https://jsfiddle.net/kmcrqfw7/2/
with hooks & keys on both nodes: https://jsfiddle.net/kmcrqfw7/3/

they all work fine.

This seems like a pretty basic tree and makes me think #164 is real.

that's not related as it appears to have too much dom created/retained rather than overly-aggressive recycling which happens here. so, basically the opposite of that issue :)

@tropperstyle
Copy link
Author

This is what I initially suspected with:

I'm guessing these mismatches are due to the dom element pooling. I tried adding _key without luck.

When you add _key https://jsfiddle.net/kmcrqfw7/4/ it only fire the remove hooks. How can I reliably access the dom after the alt content becomes available?

These fiddles were obviously trimmed down to bare bones, but in complex real world apps with large trees, I need to touch the dom manually, and placing that code in the node level hooks seemed ideal. But now I'm not even sure why you would even need these node-level hooks in your application space. Isn't it irrelevant to the app if a node is recycled, inserted, or re-inserted? That seems like a low level decision made by the library. Doesn't the app more likely care about animating the element in/out or initializing some code when the dom is ready?

So this brings me back to:

Is a sub-view required to encapsulate this type of life-cycle?

@leeoniya
Copy link
Member

leeoniya commented Jul 12, 2017

When you add _key https://jsfiddle.net/kmcrqfw7/4/ it only fire the remove hooks.

this is because keyed nodes (alt content) are allowed recycle the dom of unkeyed nodes (main content) which are no longer present in the vtree. if you key both nodes, then the recycling is prevented: https://jsfiddle.net/kmcrqfw7/5/ and your hooks will fire when the dom gets recreated. so a sub-view is not required.

How can I reliably access the dom after the alt content becomes available?

all dom elements are accessible via vnode.el and you can access specific vnodes by adding a {_ref: "blah"} to them which makes them accessible as vm.refs.blah. on each redraw vm.refs.blah will be updated with the fresh vnode, so you should not retain any vnode refs in your own code since this will cause mem leaks as they become stale on each redraw.

BTW: id, name, _ref, and _key will all act as keys [1].

But now I'm not even sure why you would even need these node-level hooks in your application space.

the primary use case is setting up css transitions. the secondary use case is embedding third-party widgets, as in http://leeoniya.github.io/domvm/demos/playground/#embed-tweets

it's hard to provide the idiomatic way to get something done when all i'm shown is a very small part of the picture. "need access to dom" needs to be qualified with "when?" and "for what?". without having this information we can talk past each other in generalizations for a long time :)

the vast majority of domvm codebases will not need hooks at all. but i can't tell you how to avoid hooks to get your thing done when i don't understand the endgame.

[1] https://github.com/leeoniya/domvm/blob/3.x-dev/src/view/initElementNode.js#L74-L81

@tropperstyle
Copy link
Author

tropperstyle commented Jul 12, 2017

the primary use case is setting up css transitions. the secondary use case is embedding third-party widgets,

But isn't this precisely what these hooks are failing at. There is no way, with pure node-level hooks, to do either of those reliably, because of the likelihood that the vnode will used a recycled dom element and therefore never have an opportunity to trigger an animation or 3rd party widget.

BTW: id, name, _ref, and _key will all act as keys [1].

What about also falling back the CSS hyperscript string?

@tropperstyle
Copy link
Author

I think my confusion most stemmed from thinking domvm provided deterministic setup and teardown hooks at the vnode level. Because most of my tree is really only distinguishable by the first hyperscript argument, a lot of recycling happens. but I think I get it now. Essentially it boils down to needing to listen to all of didInsert didReinsert and didRecycle for setup and didRecycle and didRemove for teardown.

@leeoniya
Copy link
Member

leeoniya commented Jul 12, 2017

domvm does the following during redraw:

  1. runs render() to generate the new vtree
  2. compares it to the old vtree and matches up old vnodes to new vnodes. for each vnew it finds a vold by matching keys or tags.
  3. as matches are found, it recycles the dom by reassigning vold.el -> vnew.el. and running a patch(vold, vnew) that diffs the mutates thr attrs & body. this is where will/didRecycle get called.
  4. runs the dom reconciler which walks the new vtree and the current dom, removing non-recycled dom elements, reordering recycled dom elements, inserting new dom elements at the right positions. this is when all the vnode hooks get invoked.

hopefully that illustrates why hook determinism can only be acheived by ensuring recycling determinism. you dont have to add hooks to everything, you just have to add keys to ensure the dom is not recycled non-deterministicly.

it's possible to modify domvm's step 2 to prevent the dom of any keyed vnodes from being recycled at all, which would eliminate the need to key all siblings and would add determinism to keyed vnodes (probably a good idea).

i think perhaps this was your expectation, and it's a reasonable one. i'll have a patch for this later today.

@leeoniya
Copy link
Member

leeoniya commented Jul 12, 2017

i think this is now officially fixed: https://jsfiddle.net/kmcrqfw7/7/

also tagged v3.0.3

thanks!

@tropperstyle
Copy link
Author

This helped out quite a bit. I'm running solid on 3.0.3 now. Thanks!

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

No branches or pull requests

2 participants