facebook / react

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

Bug: deleting structure via DOM API breaks state updates #30444

Open hesxenon opened 4 months ago

hesxenon commented 4 months ago

React version: 18.2.0

Steps To Reproduce

  1. have a node with a nested structure that embeds state
  2. change that nodes innerHTML, e.g. by setting it empty
  3. update the state
  4. updated state does not restore DOM Nodes

Link to code example:

https://codesandbox.io/p/sandbox/react-webcomponents-69d8v8

The current behavior

DOM stays empty, i.e. rerenders have no effect

The expected behavior

DOM should be re-populated, i.e. react should take control again.

Context

I can kind of understand why react "breaks" but the issue here is that by e.g. using webcomponents that integrate with forms and implement a "reset" behaviour the applied solution will most likely involve some kind of innerHTML setting. I wish react wouldn't just "give up" in such cases. Is there a workaround possible here?

surenpoghosian commented 3 months ago

@hesxenon The provided sandbox seems to be private or does not exist at all, please recheck.

Screenshot 2024-08-04 at 20 08 42
hesxenon commented 3 months ago

hmmm, it's set to public for me :thinking:

I'll just paste the (rather simple) setup code for reference, hopefully setting it to private and public again will have solved the issue though.

export default function App() {
  const [count, setCount] = React.useState(0);
  const ref = React.useRef<HTMLDivElement>(null);

  React.useEffect(() => {
    if (ref.current == null) {
      return;
    }
    ref.current.innerHTML = ref.current.innerHTML; // enable this and react "breaks"
  }, []);

  return (
    <div>
      <button onClick={() => setCount(count + 1)}>Inc</button>
      {count}
      <div ref={ref}>
        <div>{count}</div>
      </div>
    </div>
  );
}
surenpoghosian commented 3 months ago

@hesxenon

React uses a virtual DOM to track changes and updates only the parts of the actual DOM that have changed. When you directly set innerHTML, it bypasses React’s control, causing inconsistencies.

Setting innerHTML replaces the entire contents of the element, and React loses track of what it should update during subsequent renders.

To avoid this issue, you should let React handle the updates. If you need to perform some DOM manipulation, do it in a way that doesn’t interfere with React’s reconciliation process.

Remove the line ref.current.innerHTML = ref.current.innerHTML; and let React handle the updates. If you need to manipulate the DOM, consider other approaches that don’t conflict with React’s rendering.

import React from "react";
import "./styles.css";

export default function App() {
  const [count, setCount] = React.useState(0);
  const ref = React.useRef<HTMLDivElement>(null);

  React.useEffect(() => {
    if (ref.current) {
      // Perform any necessary DOM manipulation here without setting innerHTML directly
    }
  }, [count]); // Run effect only when count changes

  return (
    <div>
      <button onClick={() => setCount(count + 1)}>Inc</button>
      {count}
      <div ref={ref}>
        <div>{count}</div>
      </div>
    </div>
  );
}
hesxenon commented 3 months ago

I can understand why this is happening but it's easy to say "do it in a way that doesn't conflict with react" without considering that e.g. webcomponents are not (and should not) be geared towards react and might include functionality that conflicts with react in a way that isn't possible to control.

In short: I hope react doesn't become like angular - drifting away from standards and simply stating to "do it the way". (and this is starting to feel like the "niceties" encountered when using primeNG and angular/mui)

If you're saying that react won't support such cases feel free to close this a not planned, I could absolutely understand that. I just hope you won't.

surenpoghosian commented 3 months ago

@eps1lon please check this case

eps1lon commented 3 months ago

I can understand why this is happening but it's easy to say "do it in a way that doesn't conflict with react" without considering that e.g. webcomponents are not (and should not) be geared towards react and might include functionality that conflicts with react in a way that isn't possible to control.

Could you provide an example with Web Components then? It's not clear from the issue why React breaks Web Components here.

hesxenon commented 3 months ago

sure thing, here you go: codesandbox that is set to public and I just tested it in a private window. Basically this just uses a web component that resets its innerHTML on closest form reset.

I think it's a reasonable case for webcomponents to reset to some state (that might not be retrievable from its attributes!) when the closest form resets while not using the shadow dom (don't know if that makes a difference here).

eps1lon commented 3 months ago

I don't think this is possible. When React re-renders, how would it know how to reconcile the new children against what the Web component set via this.innerHTML =?

hesxenon commented 3 months ago

it wouldn't, but if I as a user of such a webcomponent am using react as a rendering solution I have to have a chance to reconcile that myself. And in the example above I'd like to have a way of saying "react, take over again"

On Mon, Aug 19, 2024, 17:13 Sebastian Silbermann @.***> wrote:

I don't think this is possible. When React re-renders, how would it know how to reconcile the new children against what the Web component set via this.innerHTML = ?

— Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/30444#issuecomment-2296820627, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACOHZTSDIXHB7EOGJTMRNDTZSIDQHAVCNFSM6AAAAABLMZ2YMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJWHAZDANRSG4 . You are receiving this because you were mentioned.Message ID: @.***>