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.

Sign up for GitHub

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

Jump to bottom

onremove() gets lost between rerenderings #1994

Closed
purplecode opened this issue Oct 15, 2017 · 7 comments
Closed

onremove() gets lost between rerenderings #1994

purplecode opened this issue Oct 15, 2017 · 7 comments

Comments

@purplecode
Copy link
Contributor

If an element is replaced by a new one, lifecycle callbacks become forgotten, and they are never called. It makes integration with third-party libraries highly complicated, because there is no easy way to do cleanups. This might be a design decision, but if so, maybe it could be better documented.

This behavior is also different than in versions < 0.2.8. Previously config function was taken into account when doing the nodes comparison and context.onunload was properly called (code below).

Expected Behavior

onremove and other lifecycle callbacks should be taken into account when doing elements comparison, and in case of any difference the node should be regarded as a different one. There is an edge case when <div oncreate={f1} /> is replaced by <div oncreate={f2} /> so both nodes have the same set of hooks, but with different function values, but some compromise could be found.

Current Behavior

In the current version onremove can be lost during rerenderings and can never be called.

<!DOCTYPE html>
<html>
<script src="https://cdnjs.cloudflare.com/ajax/libs/mithril/1.1.4/mithril.js"></script>
<body>
<div id="root"></div>
<script>
const oncreate = () => console.log('called oncreate');
const onremove = () => console.log(`called onremove`);

m.render(window.root, m('div', m('div', {oncreate, onremove})));
m.render(window.root, m('div', m('div'))); // -> onremove not called
m.render(window.root, null); // -> onremove was never called
</script>
</body>
</html>

For comparison version 0.2.8:

<!DOCTYPE html>
<html>
<script src="https://cdnjs.cloudflare.com/ajax/libs/mithril/0.2.8/mithril.js"></script>
<body>
<div id="root"></div>
<script>

const config = (element, isInitialized, context) => {
  console.log('called config');
  context.onunload = () => console.log(`called unload`);
};

m.render(window.root, m('div', m('div', {config})));
m.render(window.root, m('div', m('div'))); // -> onunload called 
</script>
</body>
</html>

Possible Solution

See expected behavior.

Steps to Reproduce (for bugs)

As in the code snippets.

Context

I tried successfully proxying hyperscript method and adding onupdate on every element, so that I can trigger the hooks manually when I detect the change, but it is very inconvenient and it disables nodes recycling.

Your Environment

  • Version used: latest
  • Browser Name and version: all
  • Operating System and version (desktop or mobile): all
@gamb
Copy link
Contributor

gamb commented Oct 15, 2017

Try:

m.render(window.root, m('div', m('div', {key: 'a', oncreate, onremove}))); // both will be called
m.render(window.root, m('div', m('div', {key: 'b'}))); // -> key identifies that this 'div' isn't the same as ^

@pygy
Copy link
Member

pygy commented Oct 15, 2017

@purplecode I think this is by design.

You can use keys or a component boundary to get the desired behavior.

One caveat with keys, they only work in list context, which is provided implicitly in many places by Mitrhil, but not everywhere. Specifically, they will be ignored when a bare, keyed vnode is returned from a view. You must wrap the returned vnode in an array.

@purplecode
Copy link
Contributor Author

Thanks, that helps.

But it basically means that key should be used with nearly every code that uses lifecycle hooks and ifs on DOM (like routing):

switch (route) {
  case '/a': render(document.body, pageA); break;
  case '/b': render(document.body, pageB); break;
}

were we have no control on what node will be replaced with.

Shall I propose a PR for documentation page? Debugging this might be a real nightmare.

@pygy
Copy link
Member

pygy commented Oct 16, 2017 •
edited

You can also wrap your pages in components. Another thing with v1 is that diff is skipped it the old vnode === the new one.

Your snippet above wouldn't be able to redraw a given route because the same vnode is fed to m.render. You must provide a fresh tree if you want it to be diffed.

You'd probably want something like this.

@dead-claudia
Copy link
Member

Closing as by design. The original snippet removed the relevant attribute hooks on the second render, then removed the node itself, so it'd be expected that they not be called. It's morally equivalent to this:

  • Add node with oncreate and onremove.
  • Remove oncreate and onremove from that node, but keep the node itself.
  • Finally, remove the node itself (it has no hooks at this point).

@purplecode
Copy link
Contributor Author

it'd be expected that they not be called

As I mentioned, when you render the whole page, with thousands of DOM nodes, you have no control on what will be replaced by what. I started using unique keys and single item arrays every time with lifecycle callbacks, but is is rather inconvenient.

But agree that there is no easy solution. My snippet with routing was just an example. In my real application we use only m.render and your vdom, not components, therefore my problem is not that common.

Since this issue still exists and can be easily mitigated by using components, maybe you could consider removal of the node-specific lifecycle callbacks in the next versions of mithril?

@dead-claudia
Copy link
Member

BTW, I've got a fix for the counter-intuitive behavior here for v2 (I might be able to backport it for v1), but it's pretty restricted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@pygy @gamb @purplecode @dead-claudia