MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 925 forks source link

Node.appendChild is not an object #1991

Closed starsolaris closed 6 years ago

starsolaris commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

starsolaris commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

starsolaris commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

starsolaris commented 7 years ago

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

starsolaris commented 7 years ago

@pygy is there any progress on this issue?

pygy commented 7 years ago

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