react: Bug: MobX-like observer pattern doesn't work with Fast Refresh because Hooks don't get detected

Let me describe the problem a bit clearer. Fast Refresh works by tagging functions with “signatures”.

// V1
function Foo() {
  useA()
}

// V2
function Foo() {
  useA()
  useB()
}

These two versions of Foo would get different “signatures”, so we’d know to remount them.

These signatures are specified through calls to a special function.

// V1
let Foo = _s(function Foo() {
  useA()  
}, 'abcde')

// V2
let Foo = _s(function Foo() {
  useA()  
  useB()  
}, 'qwerty')

The tricky part is what happens when it’s a HOC. Because the part getting signed is the inner one.

// V1
let Foo = observer(_s(function Foo() {
  useA()  
}, 'abcde'))

// V2
let Foo = observer(_s(function Foo() {
  useA()  
  useB()  
}, 'qwerty'))

This would normally work fine. React would see that the signature associated Foo is different, so all Foo components need to be remounted.

The problem with what MobX is doing is that Foo does not end up being a component in the tree. MobX just calls it as a function: Foo(). The only component React sees is the MobX generated wrapper. React has no idea that the _s call it did for Foo actually describes the signature of the wrapper that calls Foo().

React doesn’t find Foo in the tree, and it thinks that MobX wrappers are completely safe to keep mounted, since their own signatures haven’t changed.

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.

This is impacting user who uses mobx as their state management, as each of component should be wrapped with observer. I noticed that this problem only affect snowpack project, CRA are fine.

When component is wrapped in observer, fast-refresh does not work.

It was a bad word play: a mob of observables; MOBservable 😅. Since it is so fine grained in its observation, there would be gazillion observables more than when using e.g rxjs.

On Fri, 26 Mar 2021, 19:15 Dan Abramov, @.***> wrote:

I was always curious… what does “M” stand for? Michel? 😃

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/20417#issuecomment-808455583, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBHE7HE6RWMRXGEOYMTTFTMNBANCNFSM4UUBJ4DA .

Oh yes, I believe you. We have noted that mobx and React have different paths. For us React components look more like simple templates when using Mobx. That being said, your work during all these years has made our developer life so much better that a simple “thank you” will never be enough to show our appreciation. Thank you for everything : )

I get the motivation for why you’d want to structure it this way, but there is some tension here between the way React works, and the way MobX tries to make it work. Unfortunately, the issue is right at the middle of that tension.

React does not work reactively based on mutation. Like you noted, we have different paths. There’s a number of things MobX can do to try to hide it away but we need to be clear that it’ll never be fully in line with the paradigm.

Here’s a concrete example that has nothing to do with Fast Refresh. Say you start here:

export default function SignIn() {
  // read some mobx stuff
}

Then you refactor it like this:

export default Wrapper(props) {
  return <SignIn {...props} />
}

function SignIn() {
  // read some mobx stuff
}

From React’s point of view, this is a perfectly normal refactor. People do these literally every day and nothing breaks. (If we’re being pedantic, refs kind of break, but forwardRef solves that, and in longer term we’ll get rid of it and just pass refs in props, solving that too.)

But I think the withVM API would subtly break if you did that. Because now the thing getting auto-wrapped into observer would be Wrapper. Since SignIn executes lazily (rather than eagerly during parent render), it would not be “observed” at all, and changes would fail to propagate.

This illustrates that it’s a leaky abstraction, and there’s only so much you can do to hide it away. So I would encourage to always wrap things in observer directly where it’s used. This would solve the problem above and also fix Fast Refresh for you. As well as other features that rely on composition.

If it’s a problem for tests, you can mock MobX out. But something has to give here.

Hi. 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:

https://github.com/misaeldossantos/react/commit/f6eb19d8ccbab54b2daa6ec599371c8183d87fec.

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

To sum up:

• The problem @yuhongda is seeing is a bug in snowpack or some other transform step which mangles the source code.

@gaearon Eventually I find out where the problem happen. It’s because @snowpack/plugin-react-refresh use replace() to insert react-refresh code, so the $$ will be also replace by $. So that’s the reason. I have commit pull request to fix it: https://github.com/snowpackjs/snowpack/pull/3015

Also. I need to update react-refresh version in dependencies of plugin-react-refresh. So can you give me the version tag of react-refresh with this fix?

Thanks a million 😛

@AoDev observer should always be applied straight at the component definition; this makes potential refactoring issues like the shown by @gaearon more explicit and easier to detect and it prevents potential mismatches between behavior of something that is called potentially wrapped and unwrapped in e.g. a unit test, or very tricky to spot, a recursive component call. But most importantly it makes it easier to verify the important “everything that renders observables should be marked observer” easier to verify. I’ll make sure the docs on mobx side will be more explicit about this. The important realisation here is that observer is a higher order function, but not a higher order component. So your utility can take care of the injection, but not of the observer wrapping without breaking other things.

@yuhongda The issue you’re experiencing is a problem with your bundler or some transformation. The code of node_modules/react-refresh/cjs/react-refresh-runtime.development.js has these lines:

    if (typeof type === 'object' && type !== null) {
      switch (getProperty(type, '$$typeof')) {

But the code I see in the browser is:

    if (typeof type === 'object' && type !== null) {
      switch (getProperty(type, '$typeof')) { // NOTE: ONLY ONE $

This is why my fix does not work for you.

I remove all the Babel plugins, the problem is still there. Maybe it’s problem about snowpack, and I’ll try it later.

Yeah, it’s pretty wierd… I ended up ditching snowpack altogether, Now I use customized esbuild for my bundler.

Hi, can you talk more about the customized esbuild solution?

Basically I build my own bundler, first I tested simple esbuild solution using react, and modify the esbuild parameters with various esbuild plugins according to my needs, and setup chokidar to watch my src folders for changes.

When source changes, I broadcast the changes using esm-hmr protocol, currently it’s still using naive dom replacement without react-refresh. But it’s really fast, even comparable to react-refresh due to esbuild.

I use fastify to serve the files.

The most important thing is mobx and another npm package that I use works out of the box.