Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Since React Native 0.69, useEffects appear to be flushed synchronously even in React legacy mode #35778

Closed
mjmasn opened this issue Jan 5, 2023 · 12 comments
Labels
Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Needs: Triage 🔍 Resolution: PR Submitted A pull request with a fix has been provided.

Comments

@mjmasn
Copy link
Contributor

mjmasn commented Jan 5, 2023

Description

As I understand it, there was a change in React 18 to flush useEffects synchronously if they result from a render triggered by a user interaction (e.g. a button press). This was described in the React 18 changelog as:

Consistent useEffect timing: React now always synchronously flushes effect functions if the update was triggered during a discrete user input event such as a click or a keydown event. Previously, the behavior wasn't always predictable or consistent.

and I think relates to this PR: facebook/react#21150 which states:

For legacy mode, we will maintain the existing behavior, since it hasn't been reported as an issue, and we'd have to do additional work to distinguish "legacy default sync" from "discrete sync" to prevent all passive effects from being treated this way.

In React 17 the behaviour was asynchronous which allowed techniques such as calling setState then performing an expensive function inside a useEffect. This seems to be a fairly common technique in React, for example to show a spinner while fetching data (either on initial render or as the result of a filter etc. changing). For example:

export const MyComponent = () => {
  const [loading, setLoading] = useState(false);

  useEffect(() => {
    if (loading) {
      for (let i = 0; i < 1000000000; i++) {
        // Expensive function (for example filtering a large list)
      }
      setLoading(false);
    }
  }, [loading]);

  const onPress = () => setLoading(true);
  
  return (
    <TouchableOpacity onPress={onPress}>
      <Text>
        {loading ? 'Loading...' : 'Press Me'}
      </Text>
    </TouchableOpacity>
  );
}

The above works fine in React 0.68 (press button, text changes to 'Loading...', the loop iterates, text changes back to 'Press Me') but in React Native 0.69 and above the button 'hangs' in the pressed-in state until the loop has completed, and the text never changes.

The reason I think this is a bug (and specifically a React Native bug) is because this new behaviour was not supposed to affect legacy rendering mode, yet in React Native it does. We can't use new React 18 features because we can't yet upgrade to the new architecture.

On the web using createRoot in React 18 exhibits this same 'button hanging' behaviour (as expected), but on both React 17, and React 18 in legacy render mode it works fine (text changes to loading before the useEffect runs).

The questions are:

  • Is this a legit bug?
  • Is this a known issue? (haven't been able to find much searching GH and the web)
  • Is there any (global) workaround to get the old behaviour (wrapping the code inside each useEffect in setTimeout(,0) works but we'd rather not have to do that everywhere)?

Version

0.69.0+

Output of npx react-native info

System:
OS: macOS 13.0.1
CPU: (8) arm64 Apple M1 Pro
Memory: 110.97 MB / 16.00 GB
Shell: 5.8.1 - /bin/zsh
Binaries:
Node: 16.10.0 - ~/.nvm/versions/node/v16.10.0/bin/node
Yarn: 1.22.11 - ~/.nvm/versions/node/v16.10.0/bin/yarn
npm: 8.0.0 - ~/.nvm/versions/node/v16.10.0/bin/npm
Watchman: 2022.10.03.00 - /opt/homebrew/bin/watchman
Managers:
CocoaPods: 1.11.3 - /Users/mike/.rbenv/shims/pod
SDKs:
iOS SDK:
Platforms: DriverKit 22.2, iOS 16.2, macOS 13.1, tvOS 16.1, watchOS 9.1
Android SDK: Not Found
IDEs:
Android Studio: 2021.3 AI-213.7172.25.2113.9014738
Xcode: 14.2/14C18 - /usr/bin/xcodebuild
Languages:
Java: 17.0.4.1 - /usr/bin/javac
npmPackages:
@react-native-community/cli: Not Found
react: 18.0.0 => 18.0.0
react-native: 0.69.5 => 0.69.5
react-native-macos: Not Found
npmGlobalPackages:
react-native: Not Found

Steps to reproduce

See example repos below. The react native ones (Effect68/69/70) can be run as a normal RN app (npm start, npx react-native run-android etc). The react web ones (Effect170/180/181/182) use esbuild and can be run with npm run build which will run esbuild in serve mode at http://127.0.0.1:8000/ by default.

Run the app then click the button. The expected behaviour (at least in React 17 / React 18 legacy mode) is that the button text changes to 'Loading...' when pressed and reverts to 'Press Me' once the loop has run.

In the react web examples, I've left the legacy mode rendering code and imports commented out to allow quick switching between old and new.

Effect68 and Effect170 show the old behaviour
Effect69, Effect70, Effect180, Effect181, Effect182 show the new behaviour
Effect180, Effect181, Effect182 can be switched to show the old behaviour by using the legacy render function rather than createRoot

Snack, code example, screenshot, or link to a repository

https://github.com/mjmasn/Effect68 (React Native 0.68 (React 17))
https://github.com/mjmasn/Effect69 (React Native 0.69 (React 18))
https://github.com/mjmasn/Effect70 (React Native 0.70 (React 18))
https://github.com/mjmasn/Effect170 (React Web 17.0.2)
https://github.com/mjmasn/Effect180 (React Web 18.0.0)
https://github.com/mjmasn/Effect182 (React Web 18.2.0)

@mjmasn
Copy link
Contributor Author

mjmasn commented Jan 5, 2023

Videos showing React Native behaviour

React Native 0.68.2

screen-20230104-161027.mp4

React Native 0.69.5

screen-20230104-161134.mp4

React Native 0.70.6 (Hermes) - bonus issue: loop perf is really bad here

screen-20230104-161352.mp4

React Native 0.70.6 (JSC)

screen-20230104-163230.mp4

@longb1997
Copy link

longb1997 commented Mar 1, 2023

+1
with me, that only happens on Android, IOS work OK

@gmerino92
Copy link

+1, killing our app's navigation right now after upgrading from 0.68 to 0.70.5

@kelset
Copy link
Contributor

kelset commented Apr 19, 2023

Hey everyone - a quick update on this performance issue: we've just released:

Both contain a fix for how we build Hermes' artifacts that should help address this problem. Please upgrade to those versions and let us know if they help!

@mjmasn
Copy link
Contributor Author

mjmasn commented Apr 19, 2023

@kelset thanks for your reply!

The Hermes perf issue was just a side comment to the main issue really, we don't and can't use Hermes (too many issues around performance vs JSC, and limitations like the fairly low maximum number of keys an object can have that affect our app and the use of third party libraries like lodash which use large objects internally for sorting)

The main issue described above is that the "React 18 running in React 17 mode" behaviour is IMO incorrect, or at least doesn't match how pure React 17 behaves, which puts us between a rock and a hard place because we have the new behaviour which breaks our app but we can't use newer React 18 features that would help us to work around that (because we can't use the new architecture yet due to incompatibilities with other libraries).

@gmerino92
Copy link

@kelset Hi there, thanks for looking at this issue!

In our case, we are currently using Expo SDK 48 which relies on RN 0.71.3 and we're still running into performance issues, so I'm guessing whatever fix was applied on v0.70.9 didn't really made a difference. As far as v0.71.7, we'll let you know if and when Expo brings support for that and see if that changes.

@cortinico
Copy link
Contributor

Hi all,
Thanks for providing the reproducer,

I've investigated this issue and indeed, there is a regression in the behavior of useEffects starting from 0.69+. That's introduced by an interaction between the react-native and scheduler package.

We do have a tentative fix which will be coming in the next days, which will most likely be included inside 0.72 that would restore the behavior to the 0.68 one.

@Pranav-yadav Pranav-yadav added the Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. label May 22, 2023
cortinico added a commit to cortinico/react-native that referenced this issue May 22, 2023
Summary:
Fixes facebook#35778

We got reports of regressions on `useEffect` starting from 0.69+ when on Hermes.

The issue seems to be caused by a bump of the `scheduler` package from 0.20 to 0.21.
In scheduler@0.21, the method `setImmediate` gets called if available
(see facebook/react#20834). This causes React Native to use Microtasks
which ends up in changing the semantic of useEffect.

The solution is to use the Native RuntimeScheduler properly.
On Paper specifically, we never initialized it as it's effectively initialized by the
TurboModuleManagerDelegate. Here I trigger the initialization of it on Paper as well.

Changelog:
[Android] [Fixed] - Make sure the Native RuntimeScheduler is initialized on Old Arch

Reviewed By: sammy-SC

Differential Revision: D46024807

fbshipit-source-id: 7287a5a982fe660aca8fc20e9e5c6840b5e5f42b
cortinico added a commit to cortinico/react-native that referenced this issue May 22, 2023
…ebook#37517)

Summary:
Pull Request resolved: facebook#37517

Fixes facebook#35778

We got reports of regressions on `useEffect` starting from 0.69+ when on Hermes.

The issue seems to be caused by a bump of the `scheduler` package from 0.20 to 0.21.
In scheduler@0.21, the method `setImmediate` gets called if available
(see facebook/react#20834). This causes React Native to use Microtasks
which ends up in changing the semantic of useEffect.

The solution is to use the Native RuntimeScheduler properly.
On Paper specifically, we never initialized it as it's effectively initialized by the
TurboModuleManagerDelegate. Here I trigger the initialization of it on Paper as well.

Changelog:
[Android] [Fixed] - Make sure the Native RuntimeScheduler is initialized on Old Arch

Reviewed By: sammy-SC

Differential Revision: D46024807

fbshipit-source-id: da117769aaa60d1048e6ec50503c74eed6a0df3e
cortinico added a commit to cortinico/react-native that referenced this issue May 22, 2023
…ebook#37517)

Summary:
Pull Request resolved: facebook#37517

Fixes facebook#35778

We got reports of regressions on `useEffect` starting from 0.69+ when on Hermes.

The issue seems to be caused by a bump of the `scheduler` package from 0.20 to 0.21.
In scheduler@0.21, the method `setImmediate` gets called if available
(see facebook/react#20834). This causes React Native to use Microtasks
which ends up in changing the semantic of useEffect.

The solution is to use the Native RuntimeScheduler properly.
On Paper specifically, we never initialized it as it's effectively initialized by the
TurboModuleManagerDelegate. Here I trigger the initialization of it on Paper as well.

Changelog:
[Android] [Fixed] - Make sure the Native RuntimeScheduler is initialized on Old Arch

Reviewed By: sammy-SC

Differential Revision: D46024807

fbshipit-source-id: 133d6de5d42f00718092601efc8b7862eda646c9
@Pranav-yadav Pranav-yadav added the Resolution: PR Submitted A pull request with a fix has been provided. label May 22, 2023
@RohovDmytro
Copy link

Hi all, Thanks for providing the reproducer,

I've investigated this issue and indeed, there is a regression in the behavior of useEffects starting from 0.69+. That's introduced by an interaction between the react-native and scheduler package.

We do have a tentative fix which will be coming in the next days, which will most likely be included inside 0.72 that would restore the behavior to the 0.68 one.

Awesome. I will allow myself to pinpoint that the issue looks severe enough for a quite a number of people to feel the joy seeing in the the nearest release, not months later.

Thanks for picking it up and working on it.

@efstathiosntonas
Copy link

@cortinico hi, is this regression happening only on Android? I’m asking because there are some issues for iOS too and the PR states Android only. Thanks

@cortinico
Copy link
Contributor

The regression happens on both platforms, the iOS fix is forthcoming in the next days and is marked as a blocker for 0.72 stable.

kelset pushed a commit that referenced this issue May 25, 2023
)

Summary:
Pull Request resolved: #37517

Fixes #35778

We got reports of regressions on `useEffect` starting from 0.69+ when on Hermes.

The issue seems to be caused by a bump of the `scheduler` package from 0.20 to 0.21.
In scheduler@0.21, the method `setImmediate` gets called if available
(see facebook/react#20834). This causes React Native to use Microtasks
which ends up in changing the semantic of useEffect.

The solution is to use the Native RuntimeScheduler properly.
On Paper specifically, we never initialized it as it's effectively initialized by the
TurboModuleManagerDelegate. Here I trigger the initialization of it on Paper as well.

Changelog:
[Android] [Fixed] - Make sure the Native RuntimeScheduler is initialized on Old Arch

Reviewed By: sammy-SC

Differential Revision: D46024807

fbshipit-source-id: d72cd774df58410467644cddeaaf37e3c227b505
@chj-damon
Copy link

since v0.72.0 has been released, has this issue been resolved?

@cortinico
Copy link
Contributor

since v0.72.0 has been released, has this issue been resolved?

Yes this issue has been fixed in 0.72.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Needs: Triage 🔍 Resolution: PR Submitted A pull request with a fix has been provided.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants