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

useEffect is broken for React Native with JSC #14352

Closed
bvaughn opened this issue Nov 28, 2018 · 13 comments · Fixed by #14358
Closed

useEffect is broken for React Native with JSC #14352

bvaughn opened this issue Nov 28, 2018 · 13 comments · Fixed by #14358

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Nov 28, 2018

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 of scheduler 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). Since useEffect 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 the setTimeout 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 a scheduler implementation that does not require postMessage or MessageChannel, since JavaScript Core does not support either.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2018

cc @acdlite

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2018

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 scheduler to use the postMessage API). This bug can also be reproduced in react-dom if you force scheduler to use the setTimeout API.

@brunolemos
Copy link

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2018

the useEffect is one render late

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:

  1. click
  2. render 1
  3. click
  4. commit 1, render 2

@brunolemos
Copy link

brunolemos commented Nov 28, 2018

Yeah I think so, too (but better confirm)

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2018

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?

@brunolemos
Copy link

brunolemos commented Nov 28, 2018

Now I'm thinking it's not the same thing.

  • Like you said, when js debug is enabled it uses the postMessage implementation not the setTimeout one that has the delay bug;
  • I just rewrote the code from the gif and waited 5 seconds, but it did not trigger the useEffect after that, with js debug enabled

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 N and render N.
Current behavior: Press increment. Show alert with N-1 and render N.

@slorber
Copy link
Contributor

slorber commented Nov 29, 2018

hey, just want to report that the issues I encountered were on Android so both platforms are probably broken

@bvaughn bvaughn changed the title useEffect is broken for React Native (iOS) useEffect is broken for React Native with JSC Nov 29, 2018
@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 29, 2018

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.

@JasCodes
Copy link

JasCodes commented Nov 29, 2018

@bvaughn Do you think this will be resolved before new year?
I don't wanna start my new project with yuckky.. Classes 😆

Thx

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 29, 2018

I hope so! But in the meanwhile, you could probably use useLayoutEffect instead? It shouldn't be affected by this bug since it's called synchronously.

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 3, 2018

react 16.7 has a couple of alphas. The first one depends on scheduler@^0.11.0-alpha but the others all depend on scheduler@^0.12.0-alpha. I'm not sure why this is, but I see that there are some significant changes between the two scheduler alphas– and I feel like back porting my fix to 0.11.0-alpha would require extra work and testing.

So for now, I'm only going to patch fix scheduler@0.12.0-alpha.

To use useEffect in your React Native app safely, just make sure you're using react@^16.7.0-alpha.2 and scheduler@^0.12.0-alpha.3.

The patch release has now been published as scheduler@0.12.0-alpha.3

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 3, 2018

I also published the fix to scheduler@0.11.3 since it potentially impacts the edge case of react-dom ConcurrentMode and IE9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants