Hook mystery with keyed nodes #1609

Closed
pygy opened this Issue Feb 9, 2017 · 16 comments

Comments

Projects
None yet
4 participants
@pygy
Member

pygy commented Feb 9, 2017

@gyandeeps reported surprising diff behaviour yesterday in gitter...

Here's an updated jsbin with a reduced test case: http://jsbin.com/fiziricege/edit?js,console,output

For the issue to occur there must be keys on the parents, hooks on the child, and no hooks on the parent.

Enabling the hooks on left makes them run on left-1 too.

@isiahmeadows isiahmeadows added the bug label Feb 9, 2017

@isiahmeadows

This comment has been minimized.

Show comment
Hide comment
@isiahmeadows

isiahmeadows Feb 9, 2017

Collaborator

Here's a much smaller repro.

It appears all you need is to change the key on the parent, and the oncreate gets erroneously ignored on the child. Node type doesn't matter, and that may even change between renders. Identity is irrelevant here, too, in all stages.

Collaborator

isiahmeadows commented Feb 9, 2017

Here's a much smaller repro.

It appears all you need is to change the key on the parent, and the oncreate gets erroneously ignored on the child. Node type doesn't matter, and that may even change between renders. Identity is irrelevant here, too, in all stages.

@pygy pygy changed the title from Hook mystery with keyed, cached nodes to Hook mystery with keyed nodes Feb 9, 2017

@lhorie

This comment has been minimized.

Show comment
Hide comment
@lhorie

lhorie Feb 9, 2017

Member

Yeah, I'm already looking into it.

Member

lhorie commented Feb 9, 2017

Yeah, I'm already looking into it.

@pygy

This comment has been minimized.

Show comment
Hide comment
@pygy

pygy Feb 9, 2017

Member

That was fast :-)

Member

pygy commented Feb 9, 2017

That was fast :-)

@lhorie

This comment has been minimized.

Show comment
Hide comment
@lhorie

lhorie Feb 9, 2017

Member

fixed in v1.0.1

Note: you still shouldn't reuse vnodes as per http://mithril.js.org/vnodes.html#avoid-anti-patterns

Member

lhorie commented Feb 9, 2017

fixed in v1.0.1

Note: you still shouldn't reuse vnodes as per http://mithril.js.org/vnodes.html#avoid-anti-patterns

@lhorie lhorie closed this Feb 9, 2017

@lhorie

This comment has been minimized.

Show comment
Hide comment
@lhorie

lhorie Feb 9, 2017

Member

@pygy I had the fix mostly ready last night, but had to attend to my crying kid :)

Member

lhorie commented Feb 9, 2017

@pygy I had the fix mostly ready last night, but had to attend to my crying kid :)

@pygy

This comment has been minimized.

Show comment
Hide comment
@pygy

pygy Feb 9, 2017

Member

@lhorie I know the drill ;-)

But it looks like there's more: https://codepen.io/anon/pen/LxgebV?editors=0011

Using different elements, toggle twice or more, and oninit doesn't fire on the child anymore. Uncomment oncreate on the parent and it works.

Also, the class names keep on piling up on the child...

Edit, I was actually using a wrong version, but it remains the same with the [current tip(https://cdn.rawgit.com/lhorie/mithril.js/8feb6d3b399277c70bf10dc653abfe3a8d4cb2a8/mithril.js).

Member

pygy commented Feb 9, 2017

@lhorie I know the drill ;-)

But it looks like there's more: https://codepen.io/anon/pen/LxgebV?editors=0011

Using different elements, toggle twice or more, and oninit doesn't fire on the child anymore. Uncomment oncreate on the parent and it works.

Also, the class names keep on piling up on the child...

Edit, I was actually using a wrong version, but it remains the same with the [current tip(https://cdn.rawgit.com/lhorie/mithril.js/8feb6d3b399277c70bf10dc653abfe3a8d4cb2a8/mithril.js).

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Feb 9, 2017

Contributor

Thanks all for help.
Here is the find test: https://jsfiddle.net/a98q7827/3/

This exactly explains what i meant to say, for first run it runs oninit and oncreate for the first time after that it should going to cycle through onupdate and onremove only. Tho DOM nodes are getting removed.

Contributor

gyandeeps commented Feb 9, 2017

Thanks all for help.
Here is the find test: https://jsfiddle.net/a98q7827/3/

This exactly explains what i meant to say, for first run it runs oninit and oncreate for the first time after that it should going to cycle through onupdate and onremove only. Tho DOM nodes are getting removed.

@lhorie

This comment has been minimized.

Show comment
Hide comment
@lhorie

lhorie Feb 9, 2017

Member

Ok this is fixed for unkeyed children as well. If you need that specific fix now, I can publish a 1.0.2. Otherwise I'm going to pile a few more fixes before a new release.

Member

lhorie commented Feb 9, 2017

Ok this is fixed for unkeyed children as well. If you need that specific fix now, I can publish a 1.0.2. Otherwise I'm going to pile a few more fixes before a new release.

@pygy

This comment has been minimized.

Show comment
Hide comment
@pygy

pygy Feb 9, 2017

Member

@lhorie The class names still pile up...

https://codepen.io/anon/pen/LxgebV?editors=0011 The current class name is prepended to the previous ones

Member

pygy commented Feb 9, 2017

@lhorie The class names still pile up...

https://codepen.io/anon/pen/LxgebV?editors=0011 The current class name is prepended to the previous ones

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Feb 9, 2017

Contributor

@lhorie were u talking about my use case above? (sorry i think thr are 2 convo gng on in here 😄 )

Contributor

gyandeeps commented Feb 9, 2017

@lhorie were u talking about my use case above? (sorry i think thr are 2 convo gng on in here 😄 )

@lhorie

This comment has been minimized.

Show comment
Hide comment
@lhorie

lhorie Feb 9, 2017

Member

@pygy oh sorry I missed your note about class names, let me take a look

Member

lhorie commented Feb 9, 2017

@pygy oh sorry I missed your note about class names, let me take a look

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Feb 9, 2017

Contributor

@lhorie Thanks for the fix. We use mithril in production and now we are migrating to 1.0 ... But we can wait if you want to pile up fixes and then do a release. Any rough timeline?

Contributor

gyandeeps commented Feb 9, 2017

@lhorie Thanks for the fix. We use mithril in production and now we are migrating to 1.0 ... But we can wait if you want to pile up fixes and then do a release. Any rough timeline?

@lhorie lhorie reopened this Feb 9, 2017

@lhorie

This comment has been minimized.

Show comment
Hide comment
@lhorie

lhorie Feb 9, 2017

Member

@pygy turns out it's because you're memoizing the attrs. That use case isn't supported. If you move the parentHooks and childHooks into the toggle function it works

Member

lhorie commented Feb 9, 2017

@pygy turns out it's because you're memoizing the attrs. That use case isn't supported. If you move the parentHooks and childHooks into the toggle function it works

@lhorie lhorie closed this Feb 9, 2017

@pygy

This comment has been minimized.

Show comment
Hide comment
@pygy

pygy Feb 9, 2017

Member

@lhorie oooh, my bad, sorry for wasting your time with this... I wouldn't do that in real code, it was a remnant of the Object.assign({key:...}, ...) thing in the JSBin of the first post.

Member

pygy commented Feb 9, 2017

@lhorie oooh, my bad, sorry for wasting your time with this... I wouldn't do that in real code, it was a remnant of the Object.assign({key:...}, ...) thing in the JSBin of the first post.

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Feb 9, 2017

Contributor

@lhorie Your latest commit does the fix the issue but now i see another issue, here is the test plan

	o("update lifecycle methods work of recycled keyed", function() {
		var createA = o.spy()
		var updateA = o.spy()
		var removeA = o.spy()
		var createB = o.spy()
		var updateB = o.spy()
		var removeB = o.spy()

		var a = function() {
			return {tag: "div", key: 1, children: [ // remove key from here to make this test pass
				{tag: "div", attrs: {oncreate: createA, onupdate: updateA, onremove: removeA}},
			]}
		}
		var b = function() {
			return {tag: "div", key: 2, children: [
				{tag: "div", attrs: {oncreate: createB, onupdate: updateB, onremove: removeB}},
			]}
		}
		render(root, a())
		render(root, a())
		o(createA.callCount).equals(1)
		o(updateA.callCount).equals(1)
		o(removeA.callCount).equals(0)

		render(root, b())
		o(createA.callCount).equals(1)
		o(updateA.callCount).equals(1)
		o(removeA.callCount).equals(1)

		render(root, a())
		render(root, a())

		o(createA.callCount).equals(2)  // Reality: 3
		o(updateA.callCount).equals(2) // Reality: 1
		o(removeA.callCount).equals(1)
	})

I hope my expectation is correct.

Contributor

gyandeeps commented Feb 9, 2017

@lhorie Your latest commit does the fix the issue but now i see another issue, here is the test plan

	o("update lifecycle methods work of recycled keyed", function() {
		var createA = o.spy()
		var updateA = o.spy()
		var removeA = o.spy()
		var createB = o.spy()
		var updateB = o.spy()
		var removeB = o.spy()

		var a = function() {
			return {tag: "div", key: 1, children: [ // remove key from here to make this test pass
				{tag: "div", attrs: {oncreate: createA, onupdate: updateA, onremove: removeA}},
			]}
		}
		var b = function() {
			return {tag: "div", key: 2, children: [
				{tag: "div", attrs: {oncreate: createB, onupdate: updateB, onremove: removeB}},
			]}
		}
		render(root, a())
		render(root, a())
		o(createA.callCount).equals(1)
		o(updateA.callCount).equals(1)
		o(removeA.callCount).equals(0)

		render(root, b())
		o(createA.callCount).equals(1)
		o(updateA.callCount).equals(1)
		o(removeA.callCount).equals(1)

		render(root, a())
		render(root, a())

		o(createA.callCount).equals(2)  // Reality: 3
		o(updateA.callCount).equals(2) // Reality: 1
		o(removeA.callCount).equals(1)
	})

I hope my expectation is correct.

@lhorie

This comment has been minimized.

Show comment
Hide comment
@lhorie

lhorie Feb 10, 2017

Member

@gyandeeps Ok I think I know what's going on. I pushed a fix for update hooks

Member

lhorie commented Feb 10, 2017

@gyandeeps Ok I think I know what's going on. I pushed a fix for update hooks

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