Rendered fewer hooks than expected with new React Native fast refresh. 0.61

[drewhamlett]

If you create a new react native project on 0.61, and create a component wrapped with observer HOC, you get an error when hot reloading(fast refresh). The component seems to work fine(which would make sense). Everything else seems good with useLocalStore and others. Just wanted to bring it up incase others are running into it. Unfortunately I can't get anything to run online. Snack doesn't support latest react native. I'd be glad to walk someone through it or create a project if needed. Thanks!

https://facebook.github.io/react-native/blog/2019/09/18/version-0.61

[danielkcz]

Can you at least make some screenshot & show call stack of mentioned error? The hot reloading was always problematic and I wouldn't be surprised if it would be RN problem considering it's a fresh new feature.

And you can also try mobx-react-lite@next which includes experimental improvements regarding Concurrent mode if that could help in any way.

[drewhamlett]

Sure, it doesn't give a trace to mobx-react-lite though. This is with mobx-react-lite@2.0.0-alpha.2. The first fast refresh works fine. It's after that it stops working and you get this.

[danielkcz]

Sorry, but until proven it's actually a problem of this library, I am going to close this. Doesn't really sound like the case considering it works in React.

[drewhamlett]

@FredyC Is fast refresh available in React on web? I think it's only React Native for now.

[mweststrate]

It is RN only indeed Op vr 27 sep. 2019 13:41 schreef Drew Hamlett <notifications@github.com>:

…

@FredyC <https://github.com/FredyC> Is fast refresh available in React on web? I think it's only React Native for now. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#226?email_source=notifications&email_token=AAN4NBFBYZLCBBD3YSAMT7DQLX5QLA5CNFSM4I3BEQEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7YYPQQ#issuecomment-535922626>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBGXAQNW7XDMPP53EK3QLX5QLANCNFSM4I3BEQEA> .

[drewhamlett]

Could @FredyC be confusing hot reloading which works with mobx-react with fast refresh which is currently not working?

[danielkcz]

It doesn't really matter how is it called or how it works, but it's likely that fast refresh is a culprit here considering its fresh new feature and given there is no mention of mobx in the stack trace. Besides, your app works correctly without it, right?

I know that debugging RN is a pain, been there, but you need to try to dig up more information first.

[bolandrm]

I'm experiencing the same issue. The problem stops when I change:

export default observer(SignInScreen)

to:

export default SignInScreen

Also using RN fast refresh.

[danielkcz]

I would generally recommend waiting for dust from fast refresh to settle. Obviously, it's a new thing and as such it will have quirks and issues. If you are impatient, try to file the issue with RN and if they actually find the problem is on MobX side, we can do changes, but until then there is no point in posting comments here.

[drewhamlett]

I did some more testing just trying different versions of observer.

import { Reaction } from 'mobx'
import React, { useState, useEffect, useMemo, useRef } from 'react'

export function observer<P>(
  baseComponent: React.FunctionComponent<P>,
): React.FunctionComponent<P> {
  return React.memo((props) => {
    // forceUpdate 2.0
    const forceUpdate = useForceUpdate()

    const reaction = useRef<Reaction | null>(null)
    if (!reaction.current) {
      reaction.current = new Reaction(`observer`, () => {
        forceUpdate()
      })
    }

    // clean up the reaction if this component is unMount
    // useUnmount(() => reaction.current.dispose())

    // render the original component, but have the
    // reaction track the observables, so that rendering
    // can be invalidated (see above) once a dependency changes
    let rendering
    reaction.current.track(() => {
      rendering = baseComponent(props)
    })
    return rendering
  })
}

function useForceUpdate() {
  const [, setTick] = useState(1)
  return () => {
    setTick((c) => c + 1)
  }
}

function useUnmount(fn) {
  useEffect(() => fn, [])
}

And I create a store in the component like this

const App = observer(() => {
  const store = useRef(
    observable({
      name: 'Drew ',
    }),
  )

It seems to work if I don't dispose of the reaction. I've tried a bunch of different

[danielkcz]

I wouldn't be surprised if "fast refresh" would be doing something unnatural to a component lifecycle. We definitely want to keep reaction cleanup :) Besides, in 2.0 this is expanded to support Concurrent mode, so if fast refresh does something out of books, then they have to fix it. Have you actually tried to file an issue with RN about this?

[gaearon]

I think the notion that Fast Refresh is broken is a bit of a hasty conclusion. We've been using it at Facebook since June so it's pretty stable. If someone can take the Mobx source code, copy paste it into the project, and then remove things until we have a minimal reproducing state, I can look closer.

[gaearon]

One thing that's worth keeping in mind is that Fast Refresh will "free" useMemo and useCallback, and it will re-run all effects once. If you rely on useMemo and useCallback as semantics guarantees, don't — there are other things beyond Fast Refresh that can break this in the future (for example, a built-in virtualization solution).

[danielkcz]

The useMemo or useCallback is not a problem here for sure. Rerunning effects could in theory cause "something", but I cannot imagine why would React complain with that specific error message. There are no conditionals in running hooks so it feels like that error is false positive 🤷‍♂

@drewhamlett @jakst Can you please prepare that repro? It's been a while since I've fiddled with RN and I don't particularly want to dive in it again. And please, use the code from next branch. It should be way more resilient for unexpected effects run and such.

Edit: Oh, actually we have a single useCallback for the forceUpdate hook, not sure why it's there really, it's a complete waste. I doubt it could be a culprit, but it's the first step that can be tried when repro is prepared.

[bolandrm]

Here's a minimal example with the relevant mobx/mobx-react-lite code copied into the project:

https://github.com/bolandrm/rn-mobx-fast-reload-example

The mobx code could probably be minimized more, but i'm not sure if that's as important as the mobx-react-lite code (which i've minimized).

A few notes:

1. to reproduce, reload the javascript and then save a javascript file 2 times. It reproduces 100% of the time after the 2nd save.
2. i've completely eliminated useCallback and the behavior persists.
3. changing const memoComponent = memo(wrappedComponent); -> const memoComponent = wrappedComponent; causes the error to not occur anymore.
4. ANY hook can be included in the App.js component to reproduce this behavior. It does not need to be a mobx store. in this example, i've used useState. If this hook is removed, the error does not occur.

[danielkcz]

@bolandrm Thanks a lot, those are interesting findings, especially no. 3. I find it hard to believe it would be something against rules. I mean I am sure other HOCs want to introduce such optimization too.

Also, I see there is no forceUpdate, so it won't actually re-render on update. I guess it doesn't matter for a minimal repro.

[gaearon]

Thanks for a minimal repro. I will be looking at this on Monday.

[drewhamlett]

Thanks for getting that @bolandrm . The other thing that's happening

If there are no hooks: The second save with fast refresh, the component doesn't render anything.

[gaearon]

I haven't managed to run the repro project yet. I'm stuck at this:

[drewhamlett]

yarn install
cd ios && pod install
react-native run-ios

?

If you open the project in Xcode to run make sure you open the xcworkspace, not the project.
react-native run-ios will do the right thing though. I just verified it worked for me.

[gaearon]

OK I had no idea I needed to run pod install, RN didn't tell me that :P

[gaearon]

Looking at useObserver, this code:

  let rendering;
  console.log('render 1')
  reaction.current.track(() => {
    console.log('inside track 1')
    rendering = fn();
  });

sometimes logs render 1 without inside track 1.

Is that expected? Does reaction.current.track guarantee its argument to be insta-called or not?

[gaearon]

OK right that behavior makes sense. As I mentioned in #226 (comment), Fast Refresh will re-run effects on every change. Re-running effects causes mobx to dispose of the reaction. So it doesn't execute the inner function anymore, causing no hooks to be called on next render.

[gaearon]

One way you could fix this is by nulling out reaction.current in dispose:

Then the first if (!reaction.current) { check will be true on next render, and the reaction will be re-created on demand.

[danielkcz]

Well, we do already have something like that in the next branch...

mobx-react-lite/src/useObserver.ts

Line 61 in bd54c8c

reactionTrackingRef.current = null

Back then we had a fairly elaborate discussion on the topic of cleanup - facebook/react#15317 to make the Strict mode satisfied. Nothing viable came out of it so we did as best as we could.

I suppose the next step is to actually try the reproduction with full code of that next useObserver, but @drewhamlett already mentioned he tried it and it did not work either.

[gaearon]

So this is only relevant to 2.0 alpha and not to 1.x? If it's a one line fix and it affects 1.x, it would still be nice to get it out because this experience reflects badly on Fast Refresh itself, and shakes confidence in the feature.

[danielkcz]

PR made and if tests will pass, it can go out. But I still would like to know if "next" work. Can someone try that, please? And if not, we will need to update reproduction to use that code and hopefully, @gaearon can have another look.

[gaearon]

@FredyC Can you make a PR to the repro repo to use code from @next? I can look at that!

[danielkcz]

Sorry, I am fairly busy myself. Hopefully one of the guys from here can manage that.

[danielkcz]

Version 1.4.2 with the change published.

[drewhamlett]

Just to confirm again. next published on npm does not work. rendered fewer hooks than expected and blank render without a hook.

[drewhamlett]

Latest version (1.4.2) working 😄 . Thanks so much for the work! I never thought I would see everyone on this. I didn't post on react-native because I thought they would blame Mobx and then Mobx would blame react-native and it would never go anywhere. This is definitely awesome.

[mweststrate]

@FredyC I don't think you looked into mobx-react yet did you? If you did, let me know, otherwise I'll cut a release for that one tonight as well with this patch.

[danielkcz]

@mweststrate Published mobx-react@6.1.4 with updated dependency.

[mweststrate]

Such fast. Thanks Daniel!

[danielkcz]

Let's keep this open though until the "next" is figured out.

[danielkcz]

Kudos to @normano64 for spotting that the same fix is probably missing from "next". I've cut a new alpha release 2.0.0-alpha.3. Can someone please test it out in RN and let me know?

I also wonder if some kind of test can be written for this scenario. To be honest, I did not fully understand what's the problem here. If someone does, PR with a test to the "next" branch is definitely welcome.

[gaearon]

To be honest, I did not fully understand what's the problem here

Fast Refresh will re-run all effects when a component's file is edited, including the ones marked with []. If you think about it, that's the only way changes to effects' source code can propagate.

What happened was:

• useObserver disposes of the Reaction in its [] cleanup handler. It assumes [] cleanup executes only on unmount.
• Fast Refresh re-runs all effects, including the [] one. This means that we clean up the "previous" effect and set it up again.
  • This disposes the Reaction.
• During rendering, we call reaction.current.track. Remember, this Reaction was just disposed of. So MobX ignores this call and doesn't call the inner function.
  • As a result, our rendering function with a useState Hook doesn't get called.
  • As a result, we have a mismatching number of Hooks between renders.

The fix was to null out the Reaction. This ensures that on next render, the same logic that initially created a Reaction, creates it again (because it's null). So we get a new Reaction to work with, and MobX uses it.

[mweststrate]

Thanks @gaearon, this explanation is super helpful

[danielkcz]

So, no hero is going to be found who can test the next branch for this issue? I will release it by the start of the week and 1.x will become obsolete so if it doesn't work, we might end up in a similar dance like before in here.

I tried writing a test for it, but even with explanation above it's somehow over my skills.

[danielkcz]

Closing this for now. The 2.0 won't be coming out that soon yet as we need to investigate how it will work with true Concurrent mode.

[Gaafar]

@FredyC I tested the release 2.0.0-alpha.3 with React Native 0.61.5 and it seems to be working as expected in my case.

Here's what I'm testing:

• enable fast refresh
• update an observer component
• check if the component is still observing the store and able to call actions

Is there a way this fix can be cherrypicked without waiting for 2.0?

[danielkcz]

@Gaafar Uh, the 1.x should be working as well ... #226 (comment). Nothing has changed since then.

[Gaafar]

Oh, but it's not working in my case. Let me see if I can create an example to reproduce