MobX and Cannot update a component from inside the function body

[samcooke98]

Intended outcome

The linked component is a very common pattern around our (large) codebase. You have some class component, that defines a member property that accessing the parent component's props.

@observer
class Component extends React.Component {
  SubComponent = observer(() => {
    return <div>{this.props.foo}</div>;
  });

  render() {
    return (
      <div>
        <this.ChildComponent />
      </div>
    );
  }
}

I'm not entirely sure this is solveable, but thought we should open an issue nonetheless.
With the upgrade to the most recent version of react we've seen some new error

How to reproduce the issue

Codesandbox

https://codesandbox.io/s/thirsty-raman-w5pmo

https://codesandbox.io/s/eloquent-sky-sbo23

Actual outcome

Click the button, open the console, and you'll see a warning from react:

Warning: Cannot update a component from inside the function body of a different component.
    in ParentObserver (at App.js:26)

In facebook/react#18178 (comment), Dan suggests that it could be a library issue - In this case, there's a lot of mobx stuff.

Broadly it seems that clicking the button causes
the class component to update ( updateClassInstance)
This updates the observable props ( set)
This then triggers the Child component to re-render, as its reactions runs.

I do think this is also maybe one of those: 'bad patterns' that is mentioned in the thread.

Related issue: facebook/react#18178

[danielkcz]

Well, to be honest, that looks like a nice foot gun to by dynamically creating a component based on props. You are avoiding a bunch of optimizations that React provides without any good reason from what I can see. Why do you consider for example this approach as wrong?

const ChildObserver = observer(({ foo }) => {
  return <div>foo: {foo}</div>
})

@observer
class ParentObserver extends React.Component {
  render() {
    return (
      <div>
        <ChildObserver foo={this.props.foo} />
      </div>
    )
  }
}

[danielkcz]

Oh actually, it's silly altogether because that component is not even recreated, it's initialized once with the initial value of those props and it will never get updated. So I am curious what is your expected gain from this pattern of yours.

[samcooke98]

it's initialized once with the initial value of those props and it will never get updated.

I'm curious why you say this.

Mobx-react makes class components have observable props:
https://github.com/mobxjs/mobx-react/blob/master/src/observerClass.ts#L34-L39

This in turn makes the ChildComponent update, right?

Otherwise how does the count in the codesandbox increment?

[danielkcz]

Yeah, you are right, I keep forgetting about that specific perk of a class observer. Functional components don't have this.

Either way, I still don't see any benefit of your pattern over the one I inlined.

[samcooke98]

Maybe we should remove that perk of the class observers? Didn't an early release of mobx-react 6 do that?

I don't think that'd fix the issue, but at least it'd remove this footgun

[danielkcz]

No, we are certainly not going to remove that now and break a bunch of the other code. I will keep this open for now if someone else wants to chime in, but this is really such an odd pattern that I would recommend you to either stick to V5 or refactor it to something supported.

[urugator]

If we would remove this perk, ChildComponent wouldn't have a way to update itself, so it would be failing silently anyway.
Under the hood it's basically trying to call setState() of ChildComponent from within a parent component, which is forbidden.
Unfortunatelly, the warning was added just recently (as mentioned in linked issue), so it was sort of working up until now...

You can get similar effect with computed (or useMemo, you just have to specify the deps manually):

@observer
class Component extends React.Component {
  @computed
  get getChildren() {
    return <div>{this.props.foo}</div>;
  };

  render() {
    return (
      <div>
        this.getChildren()
      </div>
    );
  }
}

[danielkcz]

Alright, I guess there is no path forward here, so let's close this. Hopefully, there will be a time where component classes will be a thing of the past and these hacks won't be necessary anymore.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.