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

[Bnaya]

React version: 17.0.0

Steps To Reproduce

1. https://codesandbox.io/s/react-refresh-webpack-plugin-rendered-more-hooks-than-during-the-previous-render-issue-ezcrz?file=/src/Comp.js
2. Delete one of the hooks there

Link to code example:
https://codesandbox.io/s/react-refresh-webpack-plugin-rendered-more-hooks-than-during-the-previous-render-issue-ezcrz?file=/src/Comp.js

The current behavior

You get "Rendered more hooks than during the previous render" error

The expected behavior

Should hot reload and re-mount the component.

The source of the issue have two parts:

1. react-refresh and the bundler fails to inject signature to the component
2. When no signature apparent, react-refresh consider the components as compatible, which is not always true, as in the repro

   react/packages/react-refresh/src/ReactFreshRuntime.js

   Lines 126 to 132 in 9aca239

   	function haveEqualSignatures(prevType, nextType) {
   	const prevSignature = allSignaturesByType.get(prevType);
   	const nextSignature = allSignaturesByType.get(nextType);
   	if (prevSignature === undefined && nextSignature === undefined) {
   	return true;
   	}

I've filed an issue also for the webpack plugin: pmmmwh/react-refresh-webpack-plugin#266
Mobx related issue: mobxjs/mobx#2668

[rizrmd]

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.

[TeosVerdi]

I'd love to find a solution (or at least workaround) for this issue.
It persists on Next.js without snowpack, only using react-refresh.
See mobxjs/mobx#2794 (comment)

[rizrmd]

Potential work around for mobx is to use <Observer /> component instead of observer hoc, for example your old component:

const Component = observer(() => {
    return <div>Hello</div>
})

will become like this:

const Component = () => {
    return <Observer>{() => (
        <div>Hello</div>
    )}</Observer>
}

[TeosVerdi]

Sadly, it doesn't work in my case. I've tried arrow functions, Observer component and HOC: neither of them do the trick, the checkbox stays checked after refresh either way.

[AoDev]

@rizkyramadhan I have tried the <Observer/> approach but couldn't solve all the cases in the end.

[rizrmd]

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

[yuhongda]

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?

[AoDev]

@rizkyramadhan I dont use snowpack, nor do I know what it is. We have a "normal" setup with react-refresh-webpack-plugin without any sort of abstraction like CRA.

React Hot Loader V3 was the last version that worked properly for our mobx / react apps and we'd stayed on it if it were not for incompatibilities coming with new React versions. RHL v4 never properly worked with similar behaviour than react-refresh. Disappointing that we had a better developer experience 5 years ago.

[rizrmd]

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.

[gaearon]

react-refresh and the bundler fails to inject signature to the component

Have you looked into why?

[gaearon]

I think you want to look at these tests:

react/packages/react-refresh/src/__tests__/ReactFreshBabelPlugin-test.js

Lines 209 to 222 in 7b84dbd

	it('registers likely HOCs with inline functions', () => {
	expect(
	transform(`
	const A = forwardRef(function() {
	return <h1>Foo</h1>;
	});
	const B = memo(React.forwardRef(() => {
	return <h1>Foo</h1>;
	}));
	export default React.memo(forwardRef((props, ref) => {
	return <h1>Foo</h1>;
	}));
	`),
	).toMatchSnapshot();

Add something like the example in your repro. What does it get transformed to? Why doesn't it get the same treatment like memo?

The relevant logic is somewhere around here, probably:

react/packages/react-refresh/src/ReactFreshBabelPlugin.js

Lines 632 to 640 in 7b84dbd

	// let Foo = hoc(() => {})
	path.replaceWith(
	t.callExpression(
	sigCallID,
	createArgumentsForSignature(node, signature, path.scope),
	),
	);
	// Result: let Foo = hoc(__signature(() => {}, ...))
	}

[gaearon]

There seems to be a misunderstanding in this thread.

First, the original issue reported in #20417 (comment) no longer reproduces on CodeSandbox. I think that this was a bug in how CodeSandbox implemented Fast Refresh, which they fixed in codesandbox/codesandbox-client#5442. So it was fixed in February.

This is confirmed by #20417 (comment):

this problem only affect snowpack project, CRA are fine.

Then, the comment in #20417 (comment) has nothing to do with that problem. The linked thread is about some checkbox not working, which is not at all the symptom described in this thread. So this was incorrectly lumped together with this issue. That issue is likely just something being broken in MobX.

Then, #20417 (comment) is a new comment about some problem but it doesn't specify what the problem actually is, or offer any reproducing case. So I can't help here and I can't guess which of the problems above it refers to.

In conclusion:

• I'm going to close this issue since the original problem was solved by CodeSandbox/Snowpack.
• Mobx fast refresh issue (Next.js) mobxjs/mobx#2794 (comment) seems unrelated and there is no reason to think it has something to do with React so far.
• Bug: MobX-like observer pattern doesn't work with Fast Refresh because Hooks don't get detected #20417 (comment) does not explain what the problem is, but if there is still a problem that isn't in MobX, please file a new issue with a repro case.

Thanks!

[gaearon]

After trying more, I can repro this now. So my conclusion seems incorrect. I'll take a closer look at this...

[gaearon]

Yayyy I can repro this in CRA. This is a good sign the problem is on our side.

[AoDev]

I have this problem with a plain webpack setup. No CRA or any abstraction. If you need a repo let me know.

[gaearon]

Hmm. So the problem here seems to be is that the observer thing doesn't actually render the component passed to it. It calls it as a function. So even though React knows that inner component needs to be remounted, the inner component is nowhere in the tree. Cause it's called as a function instead of rendered as a component.

[gaearon]

This is a pretty exotic and unidiomatic pattern tbh. But it makes sense that MobX does it since it needs to know what has been read.

[gaearon]

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.

[AoDev]

Thanks for the feedback, I'll see if I can come up with a solution that can get the best of both worlds. To be honest I've been checking interesting projects like mobx-jsx but it's more an experiment than anything. It was already hard to get companies to use Mobx a couple of years ago, now asking them to not use React would be mission impossible ; )

[mweststrate]

@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.

[AoDev]

I guess I have a misunderstanding of how observer works.

Isn't export default observer(MyComponent) the same as

import MyComponent

export default observer(MyComponent)

Because that's what the utility does.

Note that this utility is mostly used for big app sections, like a root feature if you will, the other components are just plain observers.

[mweststrate]

Correct it's not the same, since 1) RHL works on a per file basis and 2) references to MyComponent might refer to the unwrapped component (for example in the MyComponent file itself, its tests, etc). This can lead to pretty unobvious bugs if there is another component in the same file, using the local MyComponent definition.

[AoDev]

Thank you both for your time. I've got my mobx-react recipes since mobservable. I probably missed something along the way. I'll move the observer decoration out and test. : )

[gaearon]

I was always curious... what does “M” stand for? Michel? :-)

[mweststrate]

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 <#20417 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBHE7HE6RWMRXGEOYMTTFTMNBANCNFSM4UUBJ4DA> .

[yuhongda]

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:
FredKSchott/snowpack#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 :P

[Bnaya]

Let me describe the problem a bit clearer. Fast Refresh works by tagging functions with "signatures".
...
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.

Sorry I'm late for the party, we are in a holiday:)
If im not mistaken, that issue can break any renderprop useage?
Is there a way to mark component as "always remount"?

[gaearon]

It don’t see how it would break render prop usage. I’d need a more specific example to explain why it doesn’t.

[gaearon]

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.

[eldh]

Solves the problem for us, when used together with FredKSchott/snowpack#3015. Thanks so much for fixing, Dan!

[Jack-Works]

If anyone using react-refresh-typescript (instead of react-refresh/babel) and having this problem, please cc me. Thanks!

[janv]

@Jack-Works I am currently dealing with this issue. I updated all of the involved libraries to their current versions as of today:

├─ @pmmmwh/react-refresh-webpack-plugin@0.5.0-rc.4
├─ react-refresh-typescript@2.0.2
├─ react-refresh@0.10.0
├─ mobx-react-lite@3.2.0
├─ mobx@6.3.2

I've been following all these discussions and still run into react complaining about mismatched hook invocations after a hot update that touches hooks in a component.

[Jack-Works]

@janv hi I'd like to fix this problem but please provide an example reproduce repository because I'm not familiar with Mobx.

[janv]

I prepared a minimal example here: https://github.com/janv/mobx-refresh-example

[Jack-Works]

I prepared a minimal example here: https://github.com/janv/mobx-refresh-example

I added hot: true, liveReload: false in your webpack config and try editing src/Component.ts, the problem doesn't occur.

[janv]

Hm, are you sure?
That doesn't seem to make a difference on my system. The error appears, with, or without the two additional config lines. And that's what I would expect, since HMR is enabled by default.

https://drive.google.com/file/d/1T1ObxHqJ8N5j0mKoAhZUaH8kEooCz853/view

[Jack-Works]

Hm, are you sure?
That doesn't seem to make a difference on my system. The error appears, with, or without the two additional config lines. And that's what I would expect, since HMR is enabled by default.

https://drive.google.com/file/d/1T1ObxHqJ8N5j0mKoAhZUaH8kEooCz853/view

Ok so the problem is HMR works, but when hooks counts differ, it doesn't recover well. Let me see why this happens

[janv]

I actually tried adding a JS component too, using the react-refresh/babel plugin (Repo has been updated).
It exhibits the same issue :/

[Jack-Works]

I actually tried adding a JS component too, using the react-refresh/babel plugin (Repo has been updated).
It exhibits the same issue :/

Hmm so can you @gaearon look at this problem?

[misaeldossantos]

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:

misaeldossantos@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