preactjs / preact

⚛️ Fast 3kB React alternative with the same modern API. Components & Virtual DOM.
https://preactjs.com
MIT License
36.53k stars 1.94k forks source link

Random wrong DOM elements order #2849

Open jedlikowski opened 3 years ago

jedlikowski commented 3 years ago

Hi there,

I'm seeing incorrect ordering of DOM elements in Firefox on Mac when rendering the below component. It's being rendered inside a Suspense and a fragment if this helps.

const CookieWidget = ({
  type = "banner",
  location = "bottom",
  onAccept = noop,
  backdrop = false,
  ...props
}) => {
  const [Widget, widgetProps] = useCookieWidget(type, location);

  return (
    <>
      {!!backdrop && <Backdrop show />}
      <Widget {...props} {...widgetProps} onAccept={onAccept} />
    </>
  );
};

This is the screenshot from rendered DOM elements: image

They should be rendered the other way around, the current order makes the backdrop cover the widget. Any help? Maybe I should provide more code parts? The weird thing is that sometimes it renders in the correct order, usually the first time after a hard refresh. Consecutive soft refreshing causes wrong order.

JoviDeCroock commented 3 years ago

Hey,

I think a codesandbox example would help us solve this quicker

xtroncode commented 3 years ago

We are also facing a similar issue while migrating to preact (random reorders) hard to reproduce.

olliekav commented 3 years ago

I'm seeing this suddenly after a Netlify deploy. I can't replicate locally and now I can't get it to work properly when building.

You can see it on https://dj.olliekav.com, the player is stuck to the top when it should be at the bottom.

The Github repo is https://github.com/olliekav/dj.olliekav.com. I'm using a context to wrap the player, and the children are being rendered in the wrong order...

https://github.com/olliekav/dj.olliekav.com/blob/8566c9be25dc722ef67ab414f580df6400bef48f/src/contexts/player-context.js#L117

return (
    <PlayerContext.Provider value={{
      player,
      wavesurfer,
      changeVolume: changeVolume,
      playTrackAtIndex: playTrackAtIndex,
      playPause: playPause,
      setTimers: setTimers
    }}>
      <div class="wrapper loaded">
        {props.children}
        <Player
          waveformChildRef={waveformRef}
        />
      </div>
    </PlayerContext.Provider>
  );
jedlikowski commented 3 years ago

In my case it turned out to be the app code loaded twice. The app I'm building is a widget people add to their sites and someone added it twice. Maybe this will help someone?

marvinhagemeister commented 3 years ago

@olliekav That's awesome, thank you so much for the detailed repro. Took a bit, but I was able to narrow it down to a ref function being side-effectful. It triggers a render and that job should only be done by any of the state setters. The faulty ref is here:

https://github.com/olliekav/dj.olliekav.com/blob/8566c9be25dc722ef67ab414f580df6400bef48f/src/contexts/player-context.js#L28-L32

const waveformRef = useCallback(node => {
  if (node !== null) {
    initWaveSurfer(node);
  }
}, []);

// Later in WaveformProgress (there are some components between here and above)
<div ref={waveformRef} />

Refs are meant to only mutate variables, and never trigger an update so the fix is to switch to useEffect for that

The fix looks like this:

const waveformRef = useRef();
useEffect(() => {
  if (waveformRef.current !== null) {
    initWaveSurfer(waveformRef.current);
  }
}, []);

// Later in WaveformProgress (there are some components between here and above)
<div ref={waveformRef} />

The code might become easier if that portion is moved into WaveformProgress directly. In the end all it does is fire up waveform and call a function passed through context and that can happen from anywhere:

// Pseudo code example for WaveformProgress
function WaveformProgress() {
  // Some sort of setter function that triggers an update
  // in the provider component.
  const { setWaveform } = useContext(PlayerContext)

  const ref = useRef();
  useEffect(() => {
    if (ref.current) {
      setWaveform(ref.current);
    }
  }, []);

  return <div ref={ref} />
}
olliekav commented 3 years ago

@marvinhagemeister wow, I really appreciate you looking at this so quickly, and giving me a code review 🙏.

I'd just moved all my class components to hooks + functional components so getting my head around the re-structuring. I just wondered why it suddenly started happening after one deploy as was fine earlier this week and I couldn't replicate it locally.

I was actually doing that to make sure the Dom node has loaded, which is in the React docs... https://reactjs.org/docs/hooks-faq.html#how-can-i-measure-a-dom-node

And yes, I was battling with forwarding refs down the component tree but I could definitely do some work on moving that inside the waveform component.

Thank you!

olliekav commented 3 years ago

@marvinhagemeister So I just made these updates, but I'm seeing the same outcome on https://dj.olliekav.com.

It's very random between hard & soft refreshes as @jedlikowski mentioned above.

olliekav commented 3 years ago

So the only way I could get the order to reliably work in my case, was to wrap the provider {props.children} in an extra container.

return (
    <PlayerContext.Provider value={{
      player,
      wavesurfer,
      changeVolume: changeVolume,
      playTrackAtIndex: playTrackAtIndex,
      playPause: playPause,
      setTimers: setTimers,
      initWavesurfer: initWavesurfer
    }}>
      <div class="wrapper loaded">
        <main class="main">
          {props.children}
        </main>
        <Player />
      </div>
    </PlayerContext.Provider>
  );

I'll put together a codepen this week and see if I can replicate with a minimal use case 👍

JoviDeCroock commented 3 years ago

Are you using createPortal for these components by any chance?

olliekav commented 3 years ago

Are you using createPortal for these components by any chance?

Nope.

JoviDeCroock commented 3 years ago

React-modal seems to be, that's where the issue presents itself

olliekav commented 3 years ago

React-modal seems to be, that's where the issue presents itself

Ah, interesting. I'll do some tests shortly with that 👍

Zhanadil1509 commented 3 years ago

i'm writing in webpack.config: resolve: { alias: { "react": "preact/compat", "react-dom": "preact/compat" } }

and random wrong DOM elements order. For example:

before: --header-- --main-- --footer--

after: --main-- --footer-- --header--

How to fix?

mikekasprzak commented 1 year ago

I'm upgrading a Preact 8 codebase to Preact X, and I also ran into this unusual DOM order behavior. I'm sharing how I worked around the issue, though it's not clear to me why the old code didn't work.

After rewriting/fixing my blog feed code, additional posts that were loaded (appended) to my feed array would be rendered in a strange order: one would get added to the end of the output, and the rest would get added to the front. If I was to console.log or view it in the Preact Dev Tools, the order would be correct. It's only the DOM that was wrong.

Anyway, like @olliekav above I solved my problem by wrapping my output in another tag (in my case, a Fragment).

render( props, state ) {
  const {feed} = state; // feed is an array of blog posts to be drawn in order. the array grows over time
  let content = [];  // an array of outputs we build below

  // Before
  //content = [...content, ...feed.map(this.makeFeedItem)];

  // After
  content.push(<Fragment>{feed.map(this.makeFeedItem)}</Fragment>);

  // This button disappears after you click it, and returns after the feed finishes loading
  if (state.isLoading) {
    content.push(<MoreButton />);
  }

  return <Fragment>{content}</Fragment>;
}

If anyone's interested, here's a written example of the unusual output.

Before clicking the MORE button, my output would look something like this.

After clicking the MORE button, the MORE button disappears (replaced with a loading animation). After loading is complete, the button returns, but output looks like this.

To me it looks to me like the last item (MoreButton) was replaced by item 5, then everything else got pushed on to the front. I did try adding keys in case that was related, but it was not.

If I put a console.log inside my makeFeedItem function, OR view the VDOM in the Preact Dev Tools, the output is what you'd expect:

Firefox and Preact 10.11.0.

albv commented 1 week ago

I'am having the same issue with Preact 10.23.2. Wrapping nodes in Fragment doesn't work in my case. Edit: it looks like a problem with createPortal, without it everything is working just fine.