Reintroduce observable state,props,context in mobx-react

[zhantx]

As described in #3858

Code change checklist

• Added/updated unit tests
• Updated /docs. For new functionality, at least API.md should be updated
• Verified that there is no significant performance drop (yarn mobx test:performance)

[changeset-bot]

🦋 Changeset detected

Latest commit: 6ff48f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
mobx-react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[zhantx]

Hi @urugator what do you think about this PR? do you think it's safe to reintroduce the observability?

[urugator]

While the global state version was the main motivation for this change, we had other reasons, some conceptual, some technical. I will share parts of our discussion:

(@urugator) Btw the setup described above - accessing this.props of parent as part of callback send to descendant - causes double renders (on current stable versions), here is a test to demonstrate the problem: https://github.com/urugator/mobx/blob/exp/packages/mobx-react/__tests__/exp.test.tsx#L27

(@urugator) Imo we should not let a change of props of current component to cause forceUpdate on other components. It's just a no-no from React perspective. It's doable by making isUpdating flag global, shared by all components (the flag that prevents notifying component itself, preventing cycle). So a reactive props can only invalidate user defined computeds/autoruns - usually some local computations in class component - this was the original motivation for reactive props.
It will break the linked test case, but that would be expected React behavior without observables - if you replace observer with memo, it would fail as well. Note there are no other observables in the test except that internal reactive props.
Just to clarify again, I am talking about current behavior, the test above runs against the current version, not the new one.

(@urugator) In short, current existing behavior is flawed:
It causes excessive renders when using render callbacks.
At the same time it allows sending referentially stable callbacks without using memoization, which probably wasn't ever intended as a feature, but people can rely on this without knowing.

(@mweststrate) I've been a bit digging in both directions, and I think we're starting to pile patches upon patches for this reactive state/props concepts, without very little benefit; and I'm inclined to instead just drop that entire concept. It has the benefit that we become much better citizens and stop hacking into React it self. Stopping that behavior basically breaks two things 1) computed values depending on props / state won't work anymore. The fix there is simple; remove the @computed annotation (and take the perf hit). The trickier problem is autoruns / reactions. I think (hope) that is a rare case. But anyway, it will be more consistent with how observer function components work, where you have to be aware as well that props / state are non reactive, and hence have to combine useEffect and reactions in the right way to make it work. So it requires some good eduction on the pitfalls and a full new major, but I think it leaves us in a much more sustainable situation.

Another problem with the implementation is that shallow equality check runs twice for props/state/context on every component update - in each setter to decide whether atom should reportChanged and then again in shouldComponentUpdate.
It's also unfortunate that the implemention affects performance of all observer components regardless of whether they need it or not and I assume majority of observers don't need it. At the same time, I think that making it configurable would lead to more confusion.

If we are to reintroduce observable props (the decision is on @mweststrate), I would at least like to see the above problems addressed:

1. render callbacks accessing this.props/state/context should not force updates during render
2. shallow check should be called only once per component update

Both can be probably workarounded, but it's some extra work and unknown problems may emerge (getting back to pilling patches point).

[zhantx]

Hi @urugator thanks for the informative reply.

render callbacks accessing this.props/state/context should not force updates during render

I tested your linked test case on my branch, it seems to me that Child does render 2 times in total which is desired as per comment, I'm not sure how to reproduce the unnecessary render (3 times) you described. In that example, Parent passes renderCallback method to Child, this won't trigger a re-render as the reference remain the same

shallow check should be called only once per component update

I agree there are some performance benefits from achieving this, I spent few hours and I couldn't achieve it in a good manner. However, (this is hacking React) that react reconciler always call shouldComponentUpdate first, then set instance.props after it, followed by setting state and context. Which means we could memorised the most recent shallowEqual call (I don't recommend this)
To us right now, removing @computed has a much bigger performance regression than extra shallowEqual

We would really appreciate it if we could make observable props behind a config, at least people like us can opt-in this feature