Stale components are not removed properly

[Nantris]

🐛 Bug Report

When re-rendering a list and adding a children, the items are added after the existing elements in the DOM, the ones that should be removed. They're transitioned out, and something is getting removed, but not everything that needs to be

To Reproduce

Repro

Expected behavior

Elements render in-place, rather than each item addition resulting in a new set of entries below the existing.

Link to repro (highly encouraged)

Repro

Environment

• react-spring v9.0.0-808.17
• react v16.12.0

[amadeus]

I'm not sure you are using transition group properly. You never remove anything from the array of items - therefore nothing ever gets removed?

[amadeus]

There's also an issue where you don't key items in the array you pass in. If you do this: https://codesandbox.io/s/epic-villani-p1dn8 then at least you start getting chunks removed

[Nantris]

@amadeus the code you provided does work better, but left running alone long enough it exhibits roughly the same behavior.

If you force opacity: 1 for all the li elements you'll see:

Pretty sure it's related to this same repro (provided by @aleclarson in another thread): https://codesandbox.io/s/react-spring-issue-927-bjrbv

I could certainly be doing something wrong. Writing code using a pre-release version means no documentation means no certainty. I don't think keys need to be passed the way you are, but I'm going off my memory from ~65 days ago. Am I wrong about the need to pass keys?

In v8 we needed something like keys={item => item.key} to define the keys, but as far as I'm aware it's not needed for v9 (which admittedly does sound wrong.)

[aleclarson]

Changing your componentDidMount method fixed the issue:

   componentDidMount() {
     setInterval(() => {
-      const { items, keyToAdd } = this.state;
-      this.setState({ keyToAdd: keyToAdd + 1 });
-      this.setState({ items: items.concat({ key: keyToAdd }) });
+      this.setState(({ items, keyToAdd }) => {
+        return {
+          keyToAdd: keyToAdd + 1,
+          items: items.concat({ key: keyToAdd }),
+        }
+      })
     }, 5000)
   }

[aleclarson]

In v8 we needed something like keys={item => item.key} to define the keys, but as far as I'm aware it's not needed for v9 (which admittedly does sound wrong.)

This section of #809 explains when the keys prop is required:

[Nantris]

Confirming resolution! Seems to work no matter how long it runs.

[Nantris]

Sad to say that when I revisited my real code for my project, it turns out we've been passing a key since before I created this issue and it does not resolve the issue.

Excuse the invalid DOM nesting. Each item is passed a unique, persistent key in the form of a UUID that should be used to remove elements from the list, but they're never removed. I really don't know what it could be besides a bug, unless I'm really misunderstanding something fundamental.

const ListRenderer = props => {
  const initial = {
    opacity: 0,
  };
  const fromProp = {
    opacity: 0,
  };
  const leave = {
    opacity: 0,
  };
  const enter = {
    opacity: 1,
  };

  const transition = useTransition(props.items, {
    initial,
    from: fromProp,
    enter,
    leave,
     config: { duration: 350 },
  });

  return transition((values, item) => 
      <animated.li key={item.props.uuid} style={values}>
        {item}
      </animated.li>
  );
};

I don't know what the issue could be, but it's definitely occurring, and as far as I can tell everything is (and long has been) configured properly, but the items continue to multiply. Some are removed, because the count of duplicated UUIDs in the DOM does sometimes go down, but it trends up on average.

Even when items.length === 0, many copies of every item remains in the DOM.

It plays out something like:

1. At startup the app immediately accumulates around 8 copies of every item in the list
2. I filter the list using the search box, the new items that still match are added, anew, briefly bumping the total copies up to 9, which then goes back down to 8 after a fraction of a second - which I assume is the item being properly removed
3. Then I clear the search box text and the 8 increases to 9 or 10, and that becomes the new baseline

All the children are React components.

[Nantris]

I've confirmed the steady growth of the elements in the list using this code:

let test = setInterval(() => {
  const el = document.querySelector('.noteList');
  console.log(el.childElementCount); // Log the number of child nodes in the list
}, 500)

As it re-renders, the count increases on average, despite some dips along the way, sort of like a boom/bust curve:

[Nantris]

Video demo

On the left, I add a character to the search field, then remove it, and repeat. On the right, the element count as determined by the code in my previous comment. At the end of the video (after a brief delay as I fumble around) I set opacity: 1 !important on the elements to show the elements that have not been unmounted.

@aleclarson any ideas?

[aleclarson]

You need expires: true for items to unmount (until I release the next canary version, where the default value is true).

[Nantris]

Thanks for your fast reply!

Unfortunately the issue persists. I tried like this I tried adding it to the hook call and the individual items, just to make sure I tried everything.

  const transition = useTransition(props.items, {
    initial,
    from: fromProp,
    enter,
    leave,
    config: { duration: 350 },
    expires: true,
  });

  return transition((values, item) => (
    <animated.li 
      key={item.props.uuid}
      // expires
      style={values}
    >
      {item}
    </animated.li>
  ));

I also tried using expires: 0 instead of true (based on now old #809) but no luck with any attempts.

At every restart of the app there seem to be a consistent 6 items in the list for each item that should be there and then more added over more re-renders.

[fozzle]

@slapbox

Thanks for your fast reply!

Unfortunately the issue persists. I tried like this I tried adding it to the hook call and the individual items, just to make sure I tried everything.

  const transition = useTransition(props.items, {
    initial,
    from: fromProp,
    enter,
    leave,
    config: { duration: 350 },
    expires: true,
  });

  return transition((values, item) => (
    <animated.li 
      key={item.props.uuid}
      // expires
      style={values}
    >
      {item}
    </animated.li>
  ));

I also tried using expires: 0 instead of true (based on now old #809) but no luck with any attempts.

At every restart of the app there seem to be a consistent 6 items in the list for each item that should be there and then more added over more re-renders.

Hello, did you ever figure out a solution to this? I'm using v9-rc3 and seeing similar behavior (dom nodes left behind after transitioning out) and trying to deduce whether it's my misuse of the API or a bug within the library 😓

[Nantris]

I haven't tried RC3 yet - but my guess is that it's not going to work. I can't reproduce it in a code sandbox, so it's not been acknowledged as a bug, so I doubt it's fixed.

I've followed everyone's suggestions and I'm always sure this next suggestion will fix it, but it never has.

Edit: Not fixed in RC3.

[fozzle]

Thanks for the update.

I've been working on reproducing in a sandbox as well, no luck yet...the actual usage in our product is unfortunately a little convoluted and thus a bit of a pain to setup in sandbox.

FWIW the issue we're seeing in product doesn't occur in 9.0.0-beta.34 but does in 9.0.0-rc.3 but between those is when the useTransition API changed...perhaps it is not related to your issue? but the bug description does match the behavior I've observed. 🤷‍♂️

I will comment back if I am able to find a repro or solution.

[Nantris]

I've been working on reproducing in a sandbox as well, no luck yet...the actual usage in our product is unfortunately a little convoluted and thus a bit of a pain to setup in sandbox.

Exactly my issue. To set it up in a sandbox would take me easily more than a day, and then if it didn't work to reproduce? I just can't afford the time it would take.

If I recall correctly I also didn't see this issue in 9.0.0-beta.34 but I had other issues.

This is where my saga began (#919), and it eventually became a different but related issue that is now this current issue (#922). It might have some bits of info related to this, but is probably not worth your time.

I'm guessing your child components are React components? Seems to be a prerequisite for producing the issue, but I can't figure out why/how to reproduce.

When I get to try the latest version I'll report back with my findings, but this is way down the priority list for us now since I've spent so much time on it already.

[amadeus]

Fwiw, I may have found the issue with stale dom nodes being left around on useTransition, see my comment here: #985 (comment)

[aleclarson]

Fixed by 513f686 (will be part of v9-rc.4)

[ignatevdev]

Is there a chance this fix would be shipped in the next few weeks? Maybe as a patch to rc-3?

It's quite a severe bug as it doesn't allow the useTransition in the current version to function properly