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.appendChild is not an object #1991

Closed
starsolaris opened this Issue Oct 14, 2017 · 15 comments

Comments

Projects
None yet
3 participants
@starsolaris
Copy link

starsolaris commented Oct 14, 2017

Mithril throw error to console

Current Behavior

TypeError: Argument 1 of Node.appendChild is not an object.

	toFragment https://unpkg.com/mithril:730:22
	updateNodes https://unpkg.com/mithril:544:59
	updateFragment https://unpkg.com/mithril:640:3
	updateNode https://unpkg.com/mithril:615:16
	updateNodes https://unpkg.com/mithril:524:12
	updateElement https://unpkg.com/mithril:674:4
	updateNode https://unpkg.com/mithril:616:15
	updateComponent https://unpkg.com/mithril:688:9
	updateNode https://unpkg.com/mithril:619:9
	updateNodes https://unpkg.com/mithril:524:12
	render https://unpkg.com/mithril:969:3
	_16/</run0 https://unpkg.com/mithril:1032:4
	throttle/< https://unpkg.com/mithril:986:4
	redraw https://unpkg.com/mithril:1014:4

Steps to Reproduce (for bugs)

Example

Your Environment

On FF 52.4.0 and Edge 15 reproduced on every run, on Chrome 61 not every run.
Windows 10

@pygy pygy self-assigned this Oct 14, 2017

@pygy pygy added the bug label Oct 14, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Oct 14, 2017

Thanks for the report. While trying to reduce this, I ended up with another weird issue:

const root = document.body
const cmp = {view({children}){return m('div', [children])}}
m.render(root, m(cmp, [
  [m('div', 1)],
  [m('div', 2)],
  [m('div', 3)]
]))
m.render(root, m(cmp, [
  [],
  [m('div', 4)]]
))
m.render(root, m(cmp, [
  [m('div', 5)]
]))
m.render(root, m(cmp, [
  [], []
]))

leaves the DOM as <div>4</div><div>5</div>.

Something is amiss in the land of nested fragments.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Oct 21, 2017

Making some progress.

Edit: Even using #1675 with the #1992 fix doesn't solve the issue if you set vnode.reuse to true. If you set it to false in oncreate, you end up with 5 3 instead of 5 4.

vnode.reuse doesn't do anyting in current Mithril, this is a feature of that PR that allows one to set a vnode marked as non-recyclable by the engine as recyclable (that occurs just before a hook that can access the DOM fires, the hooks that don't fire have no impact).

Edit again: Here's a version where reuse can be toggled on a per vnode basis.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Oct 23, 2017

Making some more progress on my sample case: replacing recycling with shouldRecycle && pool != null in the condition before insertNode makes the 4 disappear. IIUC, the only nodes that must be inserted are those that come from the pool when shouldUpdate is true...

Still that doesn't get us rid of the 5, nor does it solve @starsolaris' crash.

Edit: my bad, good news, I had not picked the right Flems to test @starsolaris' sample. replacing recycling with shouldRecycle && pool != null (the first occurrence) does solve the problem.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 23, 2017

@pygy I seriously look forward to seeing this PR...

@starsolaris

This comment has been minimized.

Copy link

starsolaris commented Oct 27, 2017

@pygy I checked your example with fix in my workflow. I cannot reproduce exception. But i still has another problem (I thought it was because of exception): Example

Mithril does not remove some nodes (green rectangle on screen)

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Oct 27, 2017

@starsolaris Thanks for verifying.

The rectangle remaining on screen probably has the same cause as the 5 not being removed in my reduction. Needs more digging (maybe this evening CET) :-)

@starsolaris

This comment has been minimized.

Copy link

starsolaris commented Oct 27, 2017

I cannot make example for now, but in next branch with fix i get new exception:

TypeError: node is null
removeNodeFromDOM@mithril.js:833:7
continuation@mithril.js:820:8
removeNode@mithril.js:810:3
removeNodes@mithril.js:789:10
updateNodes@mithril.js:623:4
updateFragment@mithril.js:668:3
updateNode@mithril.js:643:16
updateNodes@mithril.js:553:12
updateElement@mithril.js:702:4
updateNode@mithril.js:644:15
updateComponent@mithril.js:716:9
updateNode@mithril.js:647:9
updateNodes@mithril.js:553:12
render@mithril.js:1023:3
_16/</run0@mithril.js:1086:4
sync@mithril.js:1066:52
throttle/</pending<@mithril.js:1041:5
@pygy

This comment has been minimized.

Copy link
Member

pygy commented Oct 27, 2017

@starsolaris Could you try with this version that disables the pool altogether?

@starsolaris

This comment has been minimized.

Copy link

starsolaris commented Oct 27, 2017

@pygy i checked this version: no exceptions, no not removed nodes.
Also, I don't fully checked it mithril or not, but no longer leak memory in my app.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 27, 2017

@pygy For context, Inferno has the option to disable pooling, which avoids certain glitches with custom elements. We may want to investigate this option, too (if for anything, just testing).

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Oct 28, 2017

@isiahmeadows #1675 sets vnode.reuse to false for custom elements, and lets users opt in/out of the pool with conservative defaults: vnode.reuse is set to false b a before ook that can touch vnode.dom fires. This assumes that you wont touch a parent or a child node from a hook.

@starsolaris you had memory leaks otherwise?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 28, 2017

@pygy Oh okay. I forgot about that. (I've literally never used it outside Mithril core.)

@starsolaris

This comment has been minimized.

Copy link

starsolaris commented Oct 28, 2017

@pyty with the version that disables the pool, i had no memory leak for now.

@starsolaris

This comment has been minimized.

Copy link

starsolaris commented Nov 12, 2017

@pygy is there any progress on this issue?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 13, 2017

@starsolaris No progress last week, sorry. Hopefully I'll finish this in the coming days.

@pygy pygy referenced this issue Nov 22, 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 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 added a commit to pygy/mithril.js that referenced this issue Dec 4, 2017

@pygy pygy closed this in eaa9f58 Dec 4, 2017

pygy added a commit that referenced this issue 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

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

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