Lucifier129 / react-lite

An implementation of React v15.x that optimizes for small script size
MIT License
1.73k stars 100 forks source link

Error relating to vchildren #90

Closed roastlechon closed 7 years ago

roastlechon commented 7 years ago

Context is that we use:

page.route.js:168 TypeError: Cannot read property 'vchildren' of undefined
    at destroyVelem (webpack:///./~/react-lite/dist/react-lite.common.js?:363:25)
    at destroyVnode (webpack:///./~/react-lite/dist/react-lite.common.js?:150:9)
    at destroyVelem (webpack:///./~/react-lite/dist/react-lite.common.js?:367:9)
    at destroyVnode (webpack:///./~/react-lite/dist/react-lite.common.js?:150:9)
    at destroyVcomponent (webpack:///./~/react-lite/dist/react-lite.common.js?:492:5)
    at destroyVnode (webpack:///./~/react-lite/dist/react-lite.common.js?:153:9)
    at destroyVcomponent (webpack:///./~/react-lite/dist/react-lite.common.js?:492:5)
    at destroyVnode (webpack:///./~/react-lite/dist/react-lite.common.js?:153:9)
    at destroyVcomponent (webpack:///./~/react-lite/dist/react-lite.common.js?:492:5)
    at destroyVnode (webpack:///./~/react-lite/dist/react-lite.common.js?:153:9)
Lucifier129 commented 7 years ago

We need more information to reproduce this problem, some dom-element created by react-lite have been removed unexpectedly would cause this error.

roastlechon commented 7 years ago

To give a bit more context... we have a page that is rendered dynamically going route to route (SPA). Starting from one page to the other, it potentially has different components.

zemaj commented 7 years ago

I'm having the same issue. The problem is here; https://github.com/Lucifier129/react-lite/blob/44dd19e5a846a1d9dff35aef0d31edf6129da5af/src/virtual-dom.js#L348

TypeError undefined is not an object (evaluating 'vchildren.length')

function destroyVelem(velem, node) {
    let { props } = velem
    let { vchildren, childNodes } = node
    for (let i = 0, len = vchildren.length; i < len; i++) {
        destroyVnode(vchildren[i], childNodes[i])
    }
    detachRef(velem.refs, velem.ref, node)
    node.eventStore = node.vchildren = null
}

I'm having a hard time replicating the issue with code as we only see it in production a couple of times a week. As its in minified code it's near impossible to trace. It looks like destroyVelem is being called on the same node twice.

The issue could be with this line node.eventStore = node.vchildren = null; Perhaps vchildren should be set to [] instead of null?

Lucifier129 commented 7 years ago

@zemaj

The error message told that vchildren is undefined, not null.

It seems like the real-dom was not created by virtual-dom, so it lost a custom property vchildren(access a nonexistent property from an object that may get undefined).

zemaj commented 7 years ago

Ah yeah, good point.

We've added a sourcemap for the file, so next time it happens I'll be able to provide a full trace.

matthewoates commented 7 years ago

I'm able to reproduce this issue in a project of mine, and it's hosted with a source map.

As far as I an tell, react-lite is not gracefully handling removing DOM nodes that it hasn't seen before. An advertising platform creates iframe nodes within our react component, but when react-lite tries to remove the component that contains the iframe, it blows up because there's no vchildren.

I may have pushed a fix by the time you view this comment.

Repro steps:

Notice the JS error when the category switches.

matthewoates commented 7 years ago

Yep. Fix was to move all DOM nodes that are touched by the 3rd party script outside of react-lite.

Basically, I just use el.innerHTML = ... to create the content.

matthewoates commented 7 years ago

I can make a repro repo if you guys have difficulty with this, but it should be simple enough to replace a DOM node with vanilla JS in componentDidMount to surface this error.

Lucifier129 commented 7 years ago

Yeah, react-lite do not support mix real-dom to virtual-dom, except dangerouslySetInnerHTML..

matthewoates commented 7 years ago

So, is this a won't fix?

Could you simply ignore DOM nodes that react-lite hasn't created, or could that result in a memory leak?

zemaj commented 7 years ago

I think it's worthwhile to fix this, or at least add something to ignore the nodes if possible. In my case it seems like another script is modifying the dom which I don't have direct control over.

On Wed, 5 Apr 2017 at 2:19 pm, Matt notifications@github.com wrote:

So, is this a won't fix?

Could you simply ignore DOM nodes that react-lite hasn't created, or could that result in a memory leak?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Lucifier129/react-lite/issues/90#issuecomment-291749850, or mute the thread https://github.com/notifications/unsubscribe-auth/AABwvw6mo07HW_aZQsBj9JkwA2VDH3OAks5rsxY9gaJpZM4LQD0t .

-- James Peter ChargeDesk CEO & Co-Founder +1 (650) 741-4677

Lucifier129 commented 7 years ago

For keeping small, we are not going to handle the practice which is not recommended or unexpectedly, react-lite just follow the best-practice of react ;)

matthewoates commented 7 years ago

@zemaj @Lucifier129 Please see my fork https://github.com/Lucifier129/react-lite/pull/108 that resolves this issue.

zemaj commented 7 years ago

Thanks Matt!

I'd love to see this merged. It's a very lightweight solution to a complex problem.

We don't always have control over the environments our apps run in. react fails gracefully in this situation and many people are going to encounter this problem when they switch from react to react-lite

On 11 April 2017 at 10:13, Matt notifications@github.com wrote:

@zemaj https://github.com/zemaj @Lucifier129 https://github.com/Lucifier129 Please see my fork #108 https://github.com/Lucifier129/react-lite/pull/108 that resolves this issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Lucifier129/react-lite/issues/90#issuecomment-293113699, or mute the thread https://github.com/notifications/unsubscribe-auth/AABwv5wmoUAysjldgtEWNXMapT0a2DeIks5rusWdgaJpZM4LQD0t .

-- James Peter ChargeDesk CEO & Co-Founder +1 (650) 741-4677