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

`onremove` called multiple times for elements taken from recycle pool #1990

Closed
purplecode opened this Issue Oct 13, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@purplecode
Copy link
Contributor

purplecode commented Oct 13, 2017

Expected Behavior

onremove should be called only once after every oncreate

Current Behavior

onremove can be called multiple times for recycled elements. Every time when element is withdrawn from recycle pool: https://github.com/MithrilJS/mithril.js/blob/next/render/render.js#L188-L190
it is added to old list of elements without any special treatment. In case when it is not used for the second time, the onremove is fired again.

<!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 Item = (id) => {
  const oncreate = () => console.log(`oncreate: ${id}`);
  const onremove = () => console.log(`onremove: ${id}`);
  return m('div', m('div', {oncreate, onremove}, `${id}`));
}

console.log('rendering:', [1,2,3]);
m.render(window.root, m('div', Item(1), Item(2), Item(3)));
// oncreate: 1
// oncreate: 2
// oncreate: 3

console.log('rendering:', [1,2]);
m.render(window.root, m('div', Item(1), Item(2)));
// onremove: 3

console.log('rendering:', [1]);
m.render(window.root, m('div', Item(1)));
// onremove: 2
// onremove: 3 -> fired second time

console.log('rendering:', []);
m.render(window.root, m('div'));
// onremove: 1

</script>
</body>
</html>

Possible Solution

Add a flag to recycled elements, that would allow for special treatment in case when they were taken from recycle pool, but not used.

Steps to Reproduce (for bugs)

As in the code snippet.

Context

I have to set in state a flag that indicates that the element is removed for the first time after oncreate. I'd rather see that as a part of library.

Your Environment

  • Version used: master
  • Browser Name and version: all
  • Operating System and version (desktop or mobile): all

@pygy pygy added the bug label Oct 13, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Oct 13, 2017

@purplecode thanks for the report, I need to think about this a bit more (and it's getting late here). I have a pending patch that fixes other issues with the pool and that introduces a boolean flag (vnode.reuse if memory serves well), to determine whether a vnode is eligible for the pool when it is removed.

That flag is true by default and set to false before the hooks are called (except oninit).

Not sure if we can also use it for that purpose... I don't know the intricacies of updateNodes by heart and it's too late now for me to dig in.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Oct 14, 2017

@purplecode I think we can solve this by adding var oldLength = old.length before the pool is concatenated, and pass Math.min(oldEnd + 1, oldLength) to removeNodes instead of oldEnd + 1.

It fixes your repro (I've patched Mithril accordingly in that Flems), but I'll have to review the various branches to make sure it covers all bases.

@pygy pygy self-assigned this Oct 31, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Nov 21, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Nov 21, 2017

@pygy pygy referenced this issue Nov 21, 2017

Merged

Fix updateNodes() (mostly vdom diffing logic). #2021

6 of 10 tasks complete

pygy added a commit to pygy/mithril.js that referenced this issue Nov 22, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Nov 22, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Nov 22, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Nov 23, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Nov 23, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Nov 23, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Nov 23, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Dec 4, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Dec 4, 2017

@pygy pygy closed this in 8950760 Dec 4, 2017

pygy added a commit that referenced this issue Dec 4, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Dec 8, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Dec 8, 2017

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this issue Oct 12, 2018

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this issue Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment