facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.55k stars 46.78k forks source link

Bug ?: Weird reconciliation result on list elements from array with same length as the list. #19695

Closed mark-night closed 6 months ago

mark-night commented 4 years ago

React version: 16.13.1

The thing turns out to be possible something else other than what it looks like when I first post this. So I've changed the title to better reflect the issue and updated some comment to the example code below. Nothing else were modified just to keep a clear record how things are discovered.

Steps To Reproduce

  1. Run this code and check in browser what elements are destroyed or reused.
    
    import React, { useState } from 'react';
    import ReactDOM from 'react-dom';

const num = [3, 4, 5, 6]; // array serving content for list elements const loop = [1, 2, 3, 4]; // array to be mapped to generate list

const RollingNums = () => { const [cur, setCur] = React.useState(0); // control number rolling

return (

    {loop.map((value, index) => { // list item counts are fixed to loop.length // some simple CS to get continued list item let idx = index + cur; idx = idx >= 0 ? idx % num.length : ((idx % num.length) + num.length) % num.length; const target = num[idx].toString(); // get content from array num for current list element return (
  • {target}
  • ); })}
{cur}

); };

ReactDOM.render(, document.querySelector('#root'));



<!--
  Your bug will get fixed much faster if we can run your code and it doesn't
  have dependencies other than React. Issues without reproduction steps or
  code examples may be immediately closed as not actionable.
-->

[Example on codepen](https://codepen.io/mark-night/pen/bGpqvyL).

<!--
  Please provide a CodeSandbox (https://codesandbox.io/s/new), a link to a
  repository on GitHub, or provide a minimal code example that reproduces the
  problem. You may provide a screenshot of the application if you think it is
  relevant to your bug report. Here are some tips for providing a minimal
  example: https://stackoverflow.com/help/mcve.
-->

## The current behavior
Clicking on `+` destroys the first item and recreates it as the last item, while reusing all others. Clicking on `-` reuses first item (the last item from last render) and destroys/recreates all others.
**This only happens if `num` and `loop` have same length.** If `num` has more items than `loop` does, items are destroyed/reused as expected.

## The expected behavior
All items should be reused instead of being destroyed.

Although it looks perfectly fine in this simple app, everything still work. However, all class driven transitions mess up due to item destroy/recreate, and children elements underneath `<li>` will reinitialize (e.g. `<img>` reload) due to the same reason.
vkurchatkin commented 4 years ago

Run this code and check in browser what elements are destroyed or reused.

How exactly are you doing that?

mark-night commented 4 years ago

Run this code and check in browser what elements are destroyed or reused.

How exactly are you doing that?

Sorry if my description confused a bit.

The code is just a simple dummy example react app, it needs to be compiled to vanilla javascript that browser understands and be inserted into some HTML that has a DOM with id 'root'. I post the original js code just to make it easier to check.

Then in web browser render the HTML and open dev tools (like Developer Tools in Chrome), check the DOM elements tree (expanded to the li level). On every button click (either + or -), if <li> element got destroyed/created, you'll see the leading '<li>' flashed, if <li> was reused instead of being destroyed, you'll see only the class part flashed .

Quick example on codepen.

gaearon commented 4 years ago

I don't think this is correct. AFAIK browser will also "flash" moves even if nodes are the same. You really need to compare actual nodes there. Or make them uncontrolled inputs and check if the text is preserved

mark-night commented 4 years ago

I don't think this is correct. AFAIK browser will also "flash" moves even if nodes are the same. You really need to compare actual nodes there. Or make them uncontrolled inputs and check if the text is preserved

Thank you! I didn't know browser flashes on item moves too! Just tried your suggestion by replacing <li> with <input>, and it's clear that they are not destroyed at all because all texts inside input remain.

With that cleared out, a few more questions rise (I know this is not a place to ask/answer questions, but the issue still highly suspicious to be bug related so far ):

  1. In the real app where I came across with this issue, the children items were <div>s each has a <img> as its child. If num and loop are of same length, - (insert a new item as the first and move all others down) cause all images reload, while + (shift out the first and push in a new item as the last) only reload image inside the last child. Looks to me, if browser "flashes" the element (not the class part), the <img> underneath it reloads. Besides, I have transitions setup for different classes, all <div> that flashed (child <img> reloaded) ignore the transitions set for the class they were assigned (should has nothing to do with the image reloading, as size for <div> is explicitly set). Although in the previous test, texts of <input> got kept, all other clues look like the item was destroyed and recreated.

  2. Most importantly, if this is just a browser related issue, how come everything work as expected when num.length > loop.length? (- flashes only the first element and class part for all others, no image reloading, transitions work fine... just like how + does.) The issue ONLY happens when num and loop are of same length. I've tried Firefox and Chrome, both behave the same. As browser only renders whatever React provides to it, no matter if num.length is greater than or equal to loop.length, browser is only asked to render loop.length of items, right? So, looks to me, React does things differently somehow, when num.length === loop.length comparing to when num.length > loop.length. Any clues?

vkurchatkin commented 4 years ago

Flashing in DevTools is not a good indication of anything. If you want to really check if the component is remounted, you can use code like this:

React.useEffect(() => {
  console.log('mounted');

  return () => {
    console.log('unmounted');
  };
}, []);
mark-night commented 4 years ago

Flashing in DevTools is not a good indication of anything. If you want to really check if the component is remounted, you can use code like this:

React.useEffect(() => {
  console.log('mounted');

  return () => {
    console.log('unmounted');
  };
}, []);

Thanks for the hint, tried that too, no surprise, console logs shows items are not unmounted when they shouldn't be, just like what the texts in input test showed. (codesandbox)

However, this doesn't explain why items reloaded and transitions ignored when num.length === loop.length.

The thing is, why things are different (e.g. transitions ignored) when num.length === loop.length? It doesn't look like some reason from the browser part, does it?

Given all the clues, a quick solution to my example could just be to insert some dummy item in num when it happens to have same length as loop, then skip the dummy item in render. But hey, doesn't this weird result interest anyone?

mark-night commented 4 years ago

I'm not sure if this is a bug from React or not, it doesn't look like a browser issue though, no matter how many items num has, browser shouldn't even know or care.

kunukn commented 4 years ago

The reconciliation seems to work as intended because not all items are deleted or recreated.

I made a quick demo based on your example using MutationObserver to detect DOM changes.

https://codesandbox.io/s/reactjs-reconciliation-result-on-list-elements-5gw67?file=/src/index.js

Notice in the console log.

I start with the items

When I click the + button, then the UI changes to this

It will say in the console

A child node has been removed <li class="3">3</li> A child node has been added <li class="3">3</li>

It has removed the last element and added a new element at the bottom.

mark-night commented 4 years ago

@kunukn Thanks for looking into this. Sorry for the late checking back.

In your codesandbox example, if I click the - button, console gives

A child node has been removed <li class="3">3</li>
A child node has been added <li class="3">3</li>
A child node has been removed <li class="4">4</li>
A child node has been added <li class="4">4</li>
A child node has been removed <li class="5">5</li>
A child node has been added <li class="5">5</li>

This is exactly the issue I'm reporting. It seems that React first remove then recreate some items, which were recreated with the same config as before they were removed, although they maintain same config to be look like same, they are treated as new items by browser. This explains weird behavior I reported earlier, like image reloading and ignoring class transitions.

Please notice though, this only happens when the two arrays are of the same length.

Thank you again, without your setup, the issue is really hard to be described clearly. I guess this at least confirms the issue does exist? I used a quick dirty fix (extend first array to make sure it is longer than the second) in my work to avoid this temporarily, however, it would be really great if this can be fixed officially.

kunukn commented 4 years ago

I can confirm that. When starting with the items

When I click the - button, then the UI changes to this

Then I get the same as you. All items except the last were removed and re-created. Maybe this is the worst-case scenario that could happen when the keys are rotating like that. Most DOM-nodes gets removed and re-created.

mark-night commented 4 years ago

@kunukn Thanks for confirming this.

I think rotating child elements like this shouldn't be uncommon, like sliding images for example, allowing bidirectional slide is like a must. It might just not that common that element source available happen to be equal to the element counts, only then this weird issue happen.

It seems that React does things differently in this situation some how.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

ohaibbq commented 3 years ago

Thanks for filing this, @mark-night! I'm running into the same issue, however, rather than just being an implementation quirk, there are user facing repercussions to our application.

Our page content is a long list of React elements in which folk's actions result in list order changing. The expected behavior is that the scroll position remains more or less the same as the items change order.

I've updated the attached test case to demonstrate this behavior. If you click the - button while scrolled down on the page, you will see that the scroll position resets to the top of the page:

https://codesandbox.io/s/reactjs-reconciliation-result-on-list-elements-forked-mddur

This video shows an example recreation-

https://user-images.githubusercontent.com/437360/117874092-6e741e00-b255-11eb-9214-ba15dc82a3c9.mov

My expectation was that the reconciler would perform insertBefore(previouslyTheLastNode, parentNode.firstChild), rather than re-creating the list by appending each of the sibling nodes, as is shown in this (low quality) video:

https://user-images.githubusercontent.com/437360/117875729-5c937a80-b257-11eb-89c5-2702d152d653.mov

@kunukn Do you have any clues as to where we might be able to fix this in the reconciler?

samcf commented 3 years ago

I wonder if my issue is related. I'm reordering a list of elements, each of which have a static and unique identity. These elements are positioned with transform: translate(0px, ${index * height}px and transition: transform 1s ease-in-out.

When re-ordered, elements successfully animate "up" but skip their transition when moving "down" the list. As far as I can see, the elements are not re-mounted during order change.

https://user-images.githubusercontent.com/1691525/133713288-b80bb27a-c97e-478e-bf44-35c5cf82c4b7.mov

Tested with React 17.0.2

import shuffle from "lodash.shuffle";
import { useState } from "react";
import "./styles.css";

const data = [
  { id: 1, name: "Foo" },
  { id: 2, name: "Bar" },
  { id: 3, name: "Baz" }
];

export default function App() {
  const [items, setItems] = useState(data);
  return (
    <div>
      <button
        onClick={() => {
          setItems(shuffle(data));
        }}
      >
        Shuffle
      </button>
      <div style={{ position: "relative" }}>
        {items.map(({ id, name }, index) => (
          <div
            key={id}
            style={{
              position: "absolute",
              border: "1px solid #ddd",
              width: "100%",
              height: 42,
              boxSizing: "border-box",
              transition: "transform 1s ease-in-out",
              transform: `translate(${0}px, ${index * 42}px)`
            }}
          >
            {name}
          </div>
        ))}
      </div>
    </div>
  );
}
stale[bot] commented 2 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

kunukn commented 2 years ago

I wonder if my issue is related

Yes it seems related. It can be confirmed by changing the key to something unique on every render. Then no animation happens.

hoqpe commented 1 year ago

I'm working on my TransitionGroup library and this is a problem for me. For example, I'm removing a few items from a list. The component sets opacity: 0; transition: all 1s; for them. Then I shuffle the list and the items are instantly applied: opacity: 0. The transition is reset because the node was removed and re-inserted.

https://user-images.githubusercontent.com/29482190/201373446-de6fc7e6-cd98-4ae0-9091-fc02d26073bf.mov

Soberia commented 1 year ago

This renders 0123456789 on the screen. After 2s item 4 will swap with 7 but on the next render, all the items between this two (5 and 6) also replay the opacity animation which means they were removed and reinserted in the DOM.

However, if we use item's index as key (which means key never change), then the animation will not replay on the items swap.

import {useState, useEffect} from 'react';
import {createRoot} from 'react-dom/client';

function App() {
  const [item, setItem] = useState([...Array(10).keys()]);

  useEffect(() => {
    window.setTimeout(() => {
      setItem(state => {
        [state[4], state[7]] = [state[7], state[4]];
        return [...state];
      });
    }, 2e3);
  }, []);

  return (
    <>
      {item.map((item, index) => (
        <div key={item} data-id={item} className="animate-opacity">
          {item}
        </div>
      ))}
    </>
  );
}

createRoot(document.getElementById('root')!).render(<App />);
github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] commented 6 months ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!