facebook / react

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

Bug: React 18 (18.2.0) skips renders in Safari even when props change #26713

Open evanbernstein opened 1 year ago

evanbernstein commented 1 year ago

It seems that React 18 in Safari is not rendering every render call when the props have changed.

It is possible that I don't understand something about React 18's rendering, however, this is not reproducible in React 17 (or in React 18 without createRoot) or in React 18 with Firefox or Chrome, which leads me to think that this is a bug.

React version: 18.2.0

Steps To Reproduce

This behavior is not reproducible in React 17 or in React 18 with Firefox or Chrome.

Link to code example: React18SafariSkipRenderPropsChanged is a github repo that demonstrates this bug. You can clone it to reproduce it yourself.

The current behavior

Safari React 18 skips rendering some prop changes so the component misses updates that only happen once.

The expected behavior

All prop changes get rendered so that they can be recorded in state by all components if necessary in Safari.

Please let me know if you have any questions about this bug report, or the attached code repository. I'd love to help explain this bug, or learn what I'm doing wrong. Thanks!

Explanation of linked code repository

This is from React18SafariSkipRenderPropsChanged's README:

This is either a demonstration of a bug with React 18 (18.2.0) in Safari, or a demonstration of how I don't quite understand React rendering.

It seems that React 18 in Safari is not always rendering every render call, even if the props have changed. Is this a bug?

This is a very small portion of a React/Typescript/Python game that I've been coding. This is a very trimmed down demonstration of the bug.

The game displays a text log of the players' actions. These messages get sent from the server, usually one at a time. Sometimes these messages arrive in quick succession, ms apart. I don't have the game server send all of the log messages every time, only the latest message. So the log needs to retain previous messages to have them continue to be displayed to the user.

In this bug demonstration app there are two Components:

LogWithState uses useState to record the full list of messages in the Log component. This worked fine with React17's render call. When I upgraded to React18 and createRoot I noticed that log messages were sometimes missing. Recently I discovered that this is not reproducible in Firefox or Chrome, but is easily reproducible in Safari 16.4 and Safari Mobile iOS 16.4.1 (the latest versions as of 2023-04-21) both on my devices and on at least one other device.

The html for this demo has two root nodes for react. We use ReactDOM from React 17 to render into react17-root. We use createRoot and React 18's render to render into react18-root.

In the React 17 dom we render LogWithState to demonstrate that this works fine.

In the React 18 dom, we render two different components. LogWithState and LogWithoutState.

As you can hopefully see for yourself, "LogWithState - React 17" and "LogWithoutState - React 18" display all 10 of the log messages. "LogWithState - React 18" should be missing some of the logs. Below is an example screenshot on my machine:

screenshot of the bug in action

gaearon commented 1 year ago

Would you mind turning this into two CodeSandbox'es? Thanks!

evanbernstein commented 1 year ago

@gaearon I created this CodeSandbox with the code. Unfortunately, the issue does not reproduce in CodeSandbox. You can certainly view the code. Is there any chance you could test out the repo that I created on your machine?

Thanks.

evanbernstein commented 1 year ago

@gaearon My best guess is that everything runs just a bit slower in CodeSandbox and thus the renders don't get skipped, at least on my Intel Mac. You are probably a much better expert than I am.

I did just try the CodeSandbox implementation on an m1 mac, and it was not reproducible there.

nicolawebdev commented 1 year ago

True, tested on my MacBook (M1 Pro) with Safari 16.5.

draysams commented 10 months ago

Test on MacBook (M2 Pro) with Safari 16.6. Can confirm I'm facing this issue as well.

evanbernstein commented 9 months ago

With the release of macOS Sonoma imminent, I updates the packages in my linked project and tested in the latest Safari.

info Direct dependencies ├─ @types/node@16.18.54 ├─ @types/react-dom@18.2.7 └─ @types/react@18.2.23 info All dependencies ├─ @types/node@16.18.54 ├─ @types/prop-types@15.7.7 ├─ @types/react-dom@18.2.7 ├─ @types/react@18.2.23 ├─ @types/scheduler@0.16.4 └─ csstype@3.1.2

With these updates this is still reproducible with Safari Version 17.0 (19616.1.27.111.16).

brunofin commented 8 months ago

we are also affected by this bug on nextjs and react 18.2.0

fabiancarlos commented 7 months ago

Its dont render nothing, cant find why... but, i fixed temporarly on Safari mobile, with my iPhone, opening: Configuration > Safari > Advanced > Experimental Features > than activate all.

So, now renders, but, its just too absurd to ask for any user to do this.

timwuhaotian commented 7 months ago

Same happens for iOS, v11.2

Hyodori04 commented 6 months ago

If use dangerouslySetInnerHTML. It works

ajbura commented 6 months ago

It happens to me as well in safari.

But I think problem is with how batch state update breaks declarative set state notion. For example


export enum AsyncStatus {
  Idle = 'idle',
  Loading = 'loading',
  Success = 'success',
  Error = 'error',
}

export type AsyncIdle = {
  status: AsyncStatus.Idle;
};

export type AsyncLoading = {
  status: AsyncStatus.Loading;
};

export type AsyncSuccess<D> = {
  status: AsyncStatus.Success;
  data: D;
};

export type AsyncError<E = unknown> = {
  status: AsyncStatus.Error;
  error: E;
};

export type AsyncState<D, E = unknown> = AsyncIdle | AsyncLoading | AsyncSuccess<D> | AsyncError<E>;

AsyncState is state of my useAsyncCallback hook. so in safari fetch request resolves so fast(~8ms when browser has it's result cached) that react batch AsyncLoading and AsyncSuccess state, and in component A where this hook used, state update from prev AsyncSuccess to new AsyncSuccess state.

So in that component A i render a child only when result is AsyncSuccess and based on that contract that child component should unmount and re-mount when the fetch request happen as I explicitly declare AyncLoading before fetch happen and AsyncSuccess got set.

And because react skip that state, my child component (screenshot below) doesn't go though mount unmount cycle. Child component:

image

And in my child component code which depend on the mount-unmount cycle doesn't run which result in in missing UI update:

if (state.status === AsyncStatus.Idle) load();

Now i can run that load() callback in useEffect to fix this or wrap the set AyncLoading state in flushSync

but issue is actually with the how batch update breaks react declarative nature as i explicitly declare the state to be updated. I think immediate update is the correct declarative nature and batch update have to be optional and declarative with API like:

batchUpdates(() => {
//multiple set state calls
})

For more: React provides a declarative way to manipulate the UI: https://react.dev/learn/reacting-to-input-with-state#how-declarative-ui-compares-to-imperative

ajbura commented 6 months ago

@evanbernstein same ^^^ is happening with you

import React from "react";

interface LogProp {
  message: string;
  number: number;
  title: string;
  subtitle: string;
}
export const LogWithState: React.FC<{ log: LogProp }> = ({ log }) => {
  const { message, number, title, subtitle } = log;
  const [prevMessages, setMessages] = React.useState<string[]>([]);
  const [prevNumber, setNumber] = React.useState(-1);
  if (number !== prevNumber) {
    const newMessages = prevMessages.concat(message);
    setMessages(newMessages);
    setNumber(number);
  }
  return (
    <>
      <h2>{title}</h2>
      <h3>{subtitle}</h3>
      <div>
        {prevMessages.map((message, i) => (
          <div key={i}>{message}</div>
        ))}
      </div>
    </>
  );
};

your setMessages(newMessages) are getting ignored by batchUpdate and it only set the last call with prevMessage and last message

ajbura commented 6 months ago

Now i can run that load() callback in useEffect to fix this or wrap the set AyncLoading state in flushSync

i was wrong about calling load() in useEffect, I mean this is something I should do i.e calling load again if one of load dependencies changes. but that batch update makes the data problem more worse as now context data from component A and context data produces by child AuthFlowLoader component, in combination serve as wrong data for react tree down the line until that load function resolves to new data.

So, overall I have to do:

flushSync(() => {
  setState({
    status: AsyncStatus.Loading,
  });
});

before resolving async callback. And API flushSync doesn't make sense to me much, because I have to tell it to do what it suppose to do at first place, and now i am more concern about possible happening of situation like this through out the whole app.

Edited:

Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.

queueMicrotask(() => {
  flushSync(() => {
    setState({
      status: AsyncStatus.Loading,
    });
  });
});

👀

navyblue21 commented 4 months ago

Same happens for iOS v14.4, React 18.2.0

BryanChrisBrown commented 4 months ago

Also hitting this issue, safari version 17.1

github-actions[bot] commented 1 month 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!

evanbernstein commented 1 month ago

bump

klauschin commented 1 month ago

bump

alextribalworldwide commented 1 month ago

bump

khayashida919 commented 2 weeks ago

bump

khayashida919 commented 2 weeks ago

I solved it by giving the Key

<Box
          bottom="40"
          h="4rem"
          key={JSON.stringify(selectedStore)}
          position="sticky"
          w="full"
          zIndex={'docked'}
        >
fabiancarlos commented 2 days ago

I finally fixed my issue, is because i used some change of state, specifically a useEffect on the parent of my Provider/Routes. It's the React behavior, i think.

The strange is, that in Desktop works fines, but on mobile browser, just pop the UI, then less then seconds, clear all the html inside the root app element, turning a blank page.

Hyodori04 commented 2 days ago

For me it was problem of css,

My error happens only in mobile I have hover css in component but it doesn't exist in mobile. So error happens. BEFORE

 &:hover {
 }

AFTER

@media (hover: hover) {
 &:hover {
  }
}

I think many reasons of this error is related in wrong css