-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
useEffect is broken for React Native with JSC #14352
Comments
cc @acdlite |
A repro case for this bug can be found in this Gist. Note that the bug will only repro if you don't connect to a remote debugger (since connecting to Chrome will cause |
Great that you found the cause! In addition to this delay, another reported issue is that, when js debug is enabled, the useEffect is one render late: I didn't wait 5 seconds on this test so it might be related. |
I think this is actually the same thing. It would be called after 5 seconds, but you're clicking too soon– in which case, React synchronously calls it before starting on the next render (so it seems to be "late" but really, if you were logging the sequence, you'd see that it's called before the follow up render.) In other words, I think you're seeing:
|
Yeah I think so, too (but better confirm) |
What I described above was what I confirmed when investigating the RN bug report already 😄 If you think you're seeing something different, maybe you could confirm? |
Now I'm thinking it's not the same thing.
Try the code with js debug enabled: import React, { useEffect, useState } from 'react'
import { Button, View } from 'react-native'
export function App() {
const [count, setCount] = useState(0)
useEffect(
() => {
if (!count) return
alert(`New value after increment: ${count}`)
},
[count],
)
return (
<View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
<Button
onPress={() => setCount(count + 1)}
title={`Increment (${count})`}
/>
</View>
)
} Expected: Press increment. Should show alert with number |
hey, just want to report that the issues I encountered were on Android so both platforms are probably broken |
Okay. I didn't realize but RN is using JSC on Android as well, so it's not a surprise that it impacts both environments. I've updated the description of this issue. |
@bvaughn Do you think this will be resolved before new year? Thx |
I hope so! But in the meanwhile, you could probably use |
react 16.7 has a couple of alphas. The first one depends on So for now, I'm only going to patch fix To use The patch release has now been published as [email protected] |
I also published the fix to |
As reported in open source (facebook/react-native/issues/21967), the
useEffect
hook is broken for React Native when using JavaScriptCore (which affects both iOS and Android).This is because the
setTimeout
branch ofscheduler
specifies a 5000ms delay. This 5000ms is supposed to be the maximum expiration time, but in this fork it ends up being the minimum callback time (unless another state update forces us to sync-flush pending callbacks). SinceuseEffect
is passive, this means React won't call it until after a few seconds have passed.I assume we (React team) haven't noticed this because we're typically using the
postMessage
implementation, but with JavaScriptCore– React Native ends up using thesetTimeout
fork.I think this was broken by commit 4d17c3f (PR #13740) which intentionally changed the delay under the (mistaken) assumption that it would only impact unit tests.
If we want to avoid using
setTimeout
for React Native, I think we'll need to add ascheduler
implementation that does not requirepostMessage
orMessageChannel
, since JavaScript Core does not support either.The text was updated successfully, but these errors were encountered: