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

Rendered fewer hooks than expected with React Fast Refresh while using observer #2668

Closed
fortezhuo opened this issue Dec 5, 2020 · 22 comments
Labels
🐛 bug 🎁 mobx-react-lite Issue or PR related to mobx-react-lite package 👓 needs investigation

Comments

@fortezhuo
Copy link

Intended outcome:
No error occurs while update script and fast refresh triggered while using wrapped observer component

Actual outcome:

Rendered fewer hooks than expected occurs while using Fast Refresh and wrapped observer component

How to reproduce the issue:

I have created codesandbox : https://codesandbox.io/s/react-fast-refresh-test-forked-3ksts

  1. After codesandbox load completely
  2. Just copy paste script like below to trigger fast refresh
  // JUst copy React.useEffect
  React.useEffect(() => {
    console.log("a");
  }, []);

  // Paste here
  React.useEffect(() => {
    console.log("a");
  }, []);

  // Paste here again

to

  // JUst copy React.useEffect
  React.useEffect(() => {
    console.log("a");
  }, []);

  // Paste here
  React.useEffect(() => {
    console.log("a");
  }, []);

  // Paste here again
  React.useEffect(() => {
    console.log("a");
  }, []);

And error will trigger like below screenshot
image

And redo it again without "observer" and no errors will occurs. I think this problem still same with old problem mobxjs/mobx-react-lite#226

Note : I use observer wrapper in purpose to reproduce this bugs and provide simple proof

Versions
mobx : 6.0.4 latest
mobx-react-lite : 3.1.6 latest

@danielkcz danielkcz added 🎁 mobx-react-lite Issue or PR related to mobx-react-lite package 👓 needs investigation labels Dec 5, 2020
@danielkcz
Copy link
Contributor

danielkcz commented Dec 5, 2020

Wow, this is really weird. Even pasting a single additional useEffect breaks it. Notice undefined on the left.

image

I would love to say it's some issue between fast refresh and/or code sandbox, but you are right it's not happening without an observer. This is some awkward edge case it seems. I have no explanation of what's going on here.

Might be worth asking React team perhaps, I can't imagine how the can library cause anything like this.

@Bnaya Do you possibly think it could be related to some of those memory cleanups?
@urugator Some ideas?

@danielkcz
Copy link
Contributor

I had to re-read mobxjs/mobx-react-lite#226 for a reminder, but it seems that the proposed fix is still present in the code, so it's probably something else.

@fortezhuo
Copy link
Author

I also tested on my react native 0.63.x project (that use react-table and mobx-react-lite latest) also trigger error rendered fewer hooks while add React.useEffect script like sample above

And on my case, this error will occur while I use react-table with observer wrapper and add some react hooks (for trigger fast refresh) on the observed component. And here some screenshots, hope them will be useful for investigation.

image

image

@urugator
Copy link
Collaborator

urugator commented Dec 5, 2020

Firstly I would verify whether it's the same problem as before - .track() called on disposed reaction and therefore ignored: console.log(reaction.isDisposed_)
If that's the case I would look for all .dispose() calls, they should be associated with reactionTrackingRef.current = null:
https://github.com/mobxjs/mobx/search?q=dispose%28%29+path%3Apackages%2Fmobx-react-lite
This one doesn't seem to be, so I would investigate:

@Bnaya
Copy link
Member

Bnaya commented Dec 5, 2020

It occurs also before v3.1.0

@Bnaya
Copy link
Member

Bnaya commented Dec 5, 2020

TL; DR;
A minimal repro of that error without any mobxy stuff:
https://codesandbox.io/s/react-refresh-webpack-plugin-rendered-more-hooks-than-during-the-previous-render-issue-ezcrz?file=/src/Comp.js

React speculatively assumes that if the old and new component have no "signature", they can be refreshed,
which is incorrect in our case
https://github.com/facebook/react/blob/9aca239f11f31109dc1a229aa1571c2bf02f5524/packages/react-refresh/src/ReactFreshRuntime.js#L126-L132


I've debugged the issue into React's code.
When react-refresh update has 2 kinds of components updates:
staleFamilies and updatedFamilies.

staleFamilies means they will get remount, updatedFamilies will get re-render.
Components that have changes in their hooks should end up as staleFamilies and to be fully remounted.


    var needsRender = false;
    var needsRemount = false;

    if (candidateType !== null) {
      var family = resolveFamily(candidateType);

      if (family !== undefined) {
        if (staleFamilies.has(family)) {
          needsRemount = true;
        } else if (updatedFamilies.has(family)) {
          if (tag === ClassComponent) {
            needsRemount = true;
          } else {
            needsRender = true;
          }
        }
      }
    }

What goes wrong?

The component that observer produces is the wrappedComponent,
And the passed baseComponent is not real component but a render prop:
return baseComponent(props, ref);

Means that the owning component of the hooks inside baseComponent is actually wrappedComponent.
Examining the error, you can see that the offending component is our baseComponent.
Screen Shot 2020-12-05 at 22 53 38

For a reason i'm still debugging, changing the hooks inside the function passed to observer, wrappedComponent is ending up as updatedFamilies and not staleFamilies, leading to a render and not remount, thus throwing the error.

Update 1:
Seems that react-refresh / react-refresh-webpack-plugin requires some static analysis on the function body to understand that it has hooks inside, and create a "signature" for the component to know if it's refreshable or not.
Due to the fact that the actual hooks sits on a different function, it generates incorrect signature for wrappedComponent.

Update 2:
A minimal repro of that error without any mobxy stuff:
https://codesandbox.io/s/react-refresh-webpack-plugin-rendered-more-hooks-than-during-the-previous-render-issue-ezcrz?file=/src/Comp.js

@urugator
Copy link
Collaborator

urugator commented Dec 5, 2020

Hm I hope the conclusion isn't that we are breaking the rules of hooks and intercepting functional component is simply not possible...

@Bnaya
Copy link
Member

Bnaya commented Dec 6, 2020

I think it's a limitation/bug of react refresh,
The workaroud would be to somehow force remount.
I think i will later open an issue for React and the webpack plugin

@mweststrate
Copy link
Member

FWIW I run into this quite regularly in a project that doesn't even use MobX. I suspect it is HoC's that aren't handled nicely with fast-refresh.

@urugator
Copy link
Collaborator

urugator commented Dec 7, 2020

HOCs are fine, but you have to create an element, not to call the function directly. Such HOCs are impossible with hooks, unless you make some assumptions either about HOC (can't contain conditionals) or about wrapped component (doesn't contain hooks)

@Bnaya
Copy link
Member

Bnaya commented Dec 7, 2020

I have few ideas for workarounds, such as making it look like there are directly custom hooks inside wrappedComponent (it worked for the non-mbox repro)
I won't have time for it today, so feel free to work on it

@Bnaya
Copy link
Member

Bnaya commented Dec 9, 2020

React bug: facebook/react#20417
Plugin bug: pmmmwh/react-refresh-webpack-plugin#266

@mweststrate
Copy link
Member

Closing here as an issue is filed at relevant packages.

@gaearon
Copy link

gaearon commented Mar 25, 2021

Something is dubious here. The issue does not occur in Create React App, so I don't think this was a core issue in Fast Refresh itself. I think there was a flaw in the CodeSandbox integration, which got fixed in codesandbox/codesandbox-client#5442.

The React Native issue is worrying (#2668 (comment)). I would expect the React Native implementation to be correct. Can you make a standalone reproducing project I could clone? So I could look at this myself.

@urugator
Copy link
Collaborator

@gaearon

First, the original issue reported in #20417 (comment) no longer reproduces on CodeSandbox.

When I comment out useEffect, I still get Rendered fewer hooks than expected. in
https://codesandbox.io/s/react-refresh-webpack-plugin-rendered-more-hooks-than-during-the-previous-render-issue-ezcrz?file=/src/Comp.js
It does not happen if I change observerlike.js to simply return baseComponent instead of wrappedComponent.
Am I missing something?

@gaearon
Copy link

gaearon commented Mar 25, 2021

Wow. I have no idea why it didn't repro for me. I tried like 10 times. I'll reopen then. Thanks!

@gaearon
Copy link

gaearon commented Mar 25, 2021

Hmm. So the issue here is that MobX observer is not actually a HOC. It looks like you're passing a component to it, but it really calls the passed function instead of rendering it. Of course React can't then find that function anywhere in the tree.

@urugator
Copy link
Collaborator

Yes (depends on definition I quess). We need to intercept the rendering itself, can we do something about it?

@gaearon
Copy link

gaearon commented Mar 26, 2021

I have a potential fix in facebook/react#21104. I verified it fixes the crash in the original report.

facebook/react#20417 has a few more other reports of things not updating for MobX users though. Not sure if these are related, will need to take a closer look.

@gaearon
Copy link

gaearon commented Mar 30, 2021

react-refresh@0.10.0 is out with the fix. I'll file an issue against the webpack plugin to update it, but feel free to upgrade it manually to try it in your project.

@misaeldossantos
Copy link

TL; DR; A minimal repro of that error without any mobxy stuff: https://codesandbox.io/s/react-refresh-webpack-plugin-rendered-more-hooks-than-during-the-previous-render-issue-ezcrz?file=/src/Comp.js

React speculatively assumes that if the old and new component have no "signature", they can be refreshed, which is incorrect in our case https://github.com/facebook/react/blob/9aca239f11f31109dc1a229aa1571c2bf02f5524/packages/react-refresh/src/ReactFreshRuntime.js#L126-L132

I've debugged the issue into React's code. When react-refresh update has 2 kinds of components updates: staleFamilies and updatedFamilies.

staleFamilies means they will get remount, updatedFamilies will get re-render. Components that have changes in their hooks should end up as staleFamilies and to be fully remounted.


    var needsRender = false;
    var needsRemount = false;

    if (candidateType !== null) {
      var family = resolveFamily(candidateType);

      if (family !== undefined) {
        if (staleFamilies.has(family)) {
          needsRemount = true;
        } else if (updatedFamilies.has(family)) {
          if (tag === ClassComponent) {
            needsRemount = true;
          } else {
            needsRender = true;
          }
        }
      }
    }

What goes wrong?

The component that observer produces is the wrappedComponent, And the passed baseComponent is not real component but a render prop: return baseComponent(props, ref);

Means that the owning component of the hooks inside baseComponent is actually wrappedComponent. Examining the error, you can see that the offending component is our baseComponent. Screen Shot 2020-12-05 at 22 53 38

For a reason i'm still debugging, changing the hooks inside the function passed to observer, wrappedComponent is ending up as updatedFamilies and not staleFamilies, leading to a render and not remount, thus throwing the error.

Update 1: Seems that react-refresh / react-refresh-webpack-plugin requires some static analysis on the function body to understand that it has hooks inside, and create a "signature" for the component to know if it's refreshable or not. Due to the fact that the actual hooks sits on a different function, it generates incorrect signature for wrappedComponent.

Update 2: A minimal repro of that error without any mobxy stuff: https://codesandbox.io/s/react-refresh-webpack-plugin-rendered-more-hooks-than-during-the-previous-render-issue-ezcrz?file=/src/Comp.js

i think i solved this problem inside react-refresh. I disabled the next update if the component name is "observerComponent". I don't know if I should do a PR as this is a mobx specific issue and maybe the react team doesn't want to include:

misaeldossantos/react@f6eb19d.

Maybe it's possible to add a property in the component function to disable refresh in any component that needs it: Component.disableReactRefresh = true, but I'm not sure that's a good thing

@AoDev
Copy link

AoDev commented Dec 4, 2024

@misaeldossantos Did your fix to detect an observer component have been included somewhere?

I know it's an old issue, and although it's closed, I recently faced this one again, wondering if there was ever a fix for it in the end.

This doesn't seem to have fixed it: facebook/react#21104
And I saw your comment here: facebook/react#20417 (comment)

I am using

  • react-refresh": "0.14.2"
  • react: "18.3.1"
  • @pmmmwh/react-refresh-webpack-plugin": "0.5.15"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug 🎁 mobx-react-lite Issue or PR related to mobx-react-lite package 👓 needs investigation
Projects
None yet
Development

No branches or pull requests

8 participants