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

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

Node.appendChild is not an object #1991

starsolaris opened this issue Oct 14, 2017 · 15 comments
Assignees
Labels
Type: Bug For bugs and any other unexpected breakage

Comments

@starsolaris
Copy link

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 Type: Bug For bugs and any other unexpected breakage label Oct 14, 2017
@pygy
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
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
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.

@dead-claudia
Copy link
Member

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

@starsolaris
Copy link
Author

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
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
Copy link
Author

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

@pygy
Copy link
Member

pygy commented Oct 27, 2017

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

@starsolaris
Copy link
Author

@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.

@dead-claudia
Copy link
Member

@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
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?

@dead-claudia
Copy link
Member

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

@starsolaris
Copy link
Author

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

@starsolaris
Copy link
Author

@pygy is there any progress on this issue?

@pygy
Copy link
Member

pygy commented Nov 13, 2017

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

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 as completed 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
dead-claudia pushed a commit to dead-claudia/mithril.js that referenced this issue Oct 12, 2018
dead-claudia pushed a commit to dead-claudia/mithril.js that referenced this issue Oct 12, 2018
dead-claudia pushed a commit to dead-claudia/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
Labels
Type: Bug For bugs and any other unexpected breakage
Projects
None yet
Development

No branches or pull requests

3 participants