breaking with react-virtualized

[ritz078]

Hi,

whenever I try to update react-virtualized data, it throws an error

Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

error stack

VM7842:1 Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at eval (eval at evaluate (:117:21), <anonymous>:1:9)
    at Object.removeResizeListener (http://l.housing.com:4000/searchView.js:4972:7)
    at AutoSizer.componentWillUnmount (http://l.housing.com:4000/searchView.js:2366:35)
    at unmountComponent (http://l.housing.com:4000/vendor.js:4244:55)
    at unmountComponent (http://l.housing.com:4000/vendor.js:4247:20)
    at recollectNodeTree (http://l.housing.com:4000/vendor.js:4112:24)
    at removeOrphanedChildren (http://l.housing.com:4000/vendor.js:4108:63)
    at innerDiffNode (http://l.housing.com:4000/vendor.js:4105:32)
    at idiff (http://l.housing.com:4000/vendor.js:4055:279)
    at innerDiffNode (http://l.housing.com:4000/vendor.js:4101:21)

. I set up a plunker of react-virtualized with preact and its working fine on first render. This is mainly an issue with preact-compat I guess.

cc: @bvaughn - in case anything can be done from your side. This error doesn't occur if I don't change the height of the container.

[developit]

@bvaughn - curious if there's anything funky being done on your end relating to height measurement?

[developit]

Ah, I have an idea what is causing this. Preact's diff removes elements it didn't create, React's doesn't. I'll look into a reliable way of ignoring unknown children!

[bvaughn]

Hey @ritz078

I created a Plnkr a few days back that showed Preact and react-virtualized working together. Have you seen it, by chance?

https://embed.plnkr.co/48amzdx0Wkv6pja4j99j/

In Facebook onboarding all day without a laptop FYI so my responses will be few and far between unfortunately.

[ritz078]

saw it. I don't think there's any error in react-virtualized. Its a preact-compat issue. Just tagged you incase anyone else faces the same issue with RV.

btw I didn't expect your reply as I was familiar that today's your 1st day at fb. 😄

[ritz078]

@developit let me know if there is any immediate fix.

[developit]

@ritz078 I'm thinking this might be the same issue as (and thus solved by) preactjs/preact#373

[ritz078]

Actually it doesn't solve the issue. I upgraded to 6.4.0 but still getting that issue.

[developit]

Ah right, I forgot this was more related to removal of non-owned nodes. Sorry about that!

[developit]

Pretty sure I have a way to make this work. Will ping you about it once I have a beta ready.

[bvaughn]

Nice!

[developit]

[email protected] (beta) should fix this.

[andest01]

I had a similar bug with Mapbox GL. Mapbox's DOM was getting removed. Updating to [email protected] fixed this.

[developit]

Awesome. Yeah this change should fix a ton of those little things, it's been a long time coming.

[ritz078]

Sadly the issue remains. Actually I had forked react-virtualized and commented the line that caused error so I was getting no error but with RV it still exists. using v7.0.3

[bvaughn]

Hey @ritz078,

I really want RV and Preact to play nice out of the box. If there's a backwards compatible patch I could apply on the RV side to help, let me know.

If you guys are still on the 7.0 version, I'm willing to make one last 7.x bugfix release even.

[ritz078]

we are on 8. You can see the line creating the error.

I removed it and everything worked fine but I don't think that's an error on RV side.

[bvaughn]

Oh! Sorry. I mistook your 7.0.3 reference to be about RV. 🙇

Yeah, I see the error but I don't now the implications of commenting out that line. (Admittedly, I haven't been following along super closely because I've been distracted with other things recently.) If there's a safe way to handle/catch/prevent that error- I'd be willing to consider patching it on the RV side.

If Preact can handle it, better still. 😁

[developit]

Is the auto size thing part of RV, or something else?

[bvaughn]

It started off as a fork of sdecima/javascript-detect-element-resize (because that util wasn't really packaged up to be used by RV) but since then I've added several performance optimizations to it- mostly to prevent unnecessary reflows.

[bvaughn]

Maybe I should package it up as its own standalone lib that RV specifies a dependency to.

[developit]

Could do. I only mentioned it because I tried to reproduce this issue and I felt like I was missing that piece. I couldn't get RV to break :(

[bvaughn]

I couldn't get RV to break - Jason Miller

To Twitter! 😂

On Nov 19, 2016 2:55 PM, "Jason Miller" [email protected] wrote:

Could do. I only mentioned it because I tried to reproduce this issue and
I felt like I was missing that piece. I couldn't get RV to break :(

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#228 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABznXKgDK5mdDduv1TZjJvCQlEaY67Uks5q_35DgaJpZM4KY5HA
.

[ritz078]

I tried on plunker but was unable to reproduce. Commenting that line is helping but will see if there are any perf issues. There aren't any functional issues though.

[klaemo]

Unfortunately, I'm still seeing this issue with preact 7.1.0, preact-compat 3.9.3 and react-virtualized 8.7.0 (using AutoSizer exclusively).
Using the webpack alias method to swap in preact:

alias: {
  'react': 'preact-compat',
  'react-dom': 'preact-compat',
  'react-addons-css-transition-group': 'preact-css-transition-group',
  'react-addons-shallow-compare': 'shallow-compare'
}

Here's the component, I don't think I'm doing anything too weird:

import React, { PropTypes, Component } from 'react'
import AutoSizer from 'react-virtualized/dist/commonjs/AutoSizer'
import classNames from 'classnames'

class Votes extends Component {
  render (props) {
    const { possibleAnswers, voteClickHandler } = props
    const count = possibleAnswers.length
    const oddOrEven = count % 2 !== 0 || count === 2 ? 'odd' : 'even'
    const votesClass = classNames('widget-votes', oddOrEven, `votes-${count}`)

    return (
      <section className={votesClass}>
        <AutoSizer disableWidth>
          {({ height }) => {
            const cellHeight = oddOrEven === 'odd' ? (height / count) : (height / (count / 2))

            return possibleAnswers.map((answer) => (
              <div key={answer.id} className='vote-container' style={{ height: cellHeight }}>
                <button
                  title={answer.text}
                  onClick={() => voteClickHandler(answer.id)}
                  className='vote-btn'
                >
                  <div className='vote-btn-text'>{answer.text}</div>
                  <div className='vote-btn-fade' />
                </button>
              </div>
            ))
          }}
        </AutoSizer>
      </section>
    )
  }
}

[developit]

I wonder if AutoSizer is actually moving the element? That would be an issue for sure..

[bvaughn]

AutoSizer is built on top of a modified (for performance) version of the detect-element-resize library. You can see the DOM manipulation it's doing here.

Let me know if you'd like to chat about it.

[ritz078]

So continuing here. this fixes it.

Any other error after removing this ? Not so far. Have been testing internally since more than a month.

Perf issues ? Doesn't seem so. (but maybe I missed something ? monitoring.)

[bvaughn]

Oh man! If that's the only error- let's get that commit merged into RV and I'll put out a point release.

Looks like you guys are based off of 8.2.0. RV main is on 8.9.0 now so the fix would go out as 8.9.1 - but if it's important for you to stay on 8.2.0 I could release an 8.2.1 fix as well.

[ritz078]

We shouldn't have any problem upgrading. Will get this tested by tomorrow or maybe you can do a beta release and I can test it.

[developit]

brian that technique for unsetting the element reference is my favorite

[bvaughn]

FYI this change has been applied to react-virtualized with 78910b6 and released as 8.11.0.

[bvaughn]

Think we can close this issue now?

[ritz078]

I think we can. 👏 we are upgrading now and will comment here if something doesn't work.

[bvaughn]

Fantastic. Best of luck

…

On Jan 16, 2017 10:51 PM, "Ritesh Kumar" ***@***.***> wrote: I think we can. 👏 we are upgrading now and will comment here if something doesn't work. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#228 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABznTdc03eHQBbAYyblswXYDJca-fVJks5rTGUCgaJpZM4KY5HA> .

[developit]

you guys are the best