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

Reintroduce observable state,props,context in mobx-react #3863

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jzhan-canva
Copy link
Contributor

@jzhan-canva jzhan-canva commented Apr 26, 2024

As requested in #3858
This PR re-introduce the observable state, props, context. It is proven to be safe to restore this feature as the bug caused by setter is now gone.

Regarding two requests from @urugator

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

This does seem to happen according to exp.test.js

shallow check should be called only once per component update

This is now solved by storing shouldComponentUpdate result in a WeakSet, so we don't re compare in setter

Next: I solved the class component tearing by wrapping class component in a function component, then utilise useSyncExternalStore. I decided to keep it in separate PR after this is merged

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)

Copy link

changeset-bot bot commented Apr 26, 2024

🦋 Changeset detected

Latest commit: 3a8c951

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jzhan-canva
Copy link
Contributor Author

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

@urugator
Copy link
Collaborator

urugator commented May 7, 2024

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

@jzhan-canva
Copy link
Contributor Author

jzhan-canva commented May 8, 2024

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

@jzhan-canva
Copy link
Contributor Author

jzhan-canva commented May 23, 2024

hello @urugator

I made some progress

I made several changes to ObserverClass

  1. as discussed above, reintroducing observable props state context
  2. solved extra shallowEqual calls. because I stop using forceUpdate API (I will explain why I stop using this API in no.4), so shouldComponentUpdate is always called. During shouldComponentUpdate, there is a weakSet to store what have been compared, so in setter we don't need to re-compare them
  3. also in shouldComponentUpdate, I also shallow compare the state because that's how react is doing
  4. wrap the class component in a function component, so we can use useSyncExternalStore to schedule the update in SyncLane, this solves the tearing between class observer and function observer

however no.4 brings few tradeoffs

  1. the static properties of class component are no longer accessible, this can be solved by re-injecting the static properties into new function component (hacky)
  2. ObserverClass itself can't detect "Redeclaring an existing observer component as an observer should throw", we need to move the logic to observer.tsx
  3. observer class can't be extended (as it's a function now)
  4. observer can only be used on Component, not PureComponent, this isn't a big issue as we always do shallow compare

@jzhan-canva
Copy link
Contributor Author

jzhan-canva commented May 23, 2024

The main logic change is to remove admin.isUpdating and add admin.pendingStateVersion and admin.stateVersion

when admin.pendingStateVersion is assigned a new symbol value, i.e. admin.pendingStateVersion !== admin.stateVersion. it means an update is scheduled or in progress (either scheduled by react or by admin.scheduleUpdate)

during the code you can see several admin.pendingStateVersion = Symbol() to make sure we are not rescheduling redundant updates. only at the end of render function it will assign admin.stateVersion = admin.pendingStateVersion

@@ -513,7 +509,7 @@ describe("should render component even if setState called with exactly the same
act(() => clickableDiv.click())
act(() => clickableDiv.click())

expect(renderCount).toBe(3)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state is shallow compared now, renderCount should be 2

@jzhan-canva jzhan-canva changed the title Reintroduce observable state,props,context in mobx-react Reintroduce observable state,props,context in mobx-react. solve tearing May 24, 2024
@jzhan-canva
Copy link
Contributor Author

jzhan-canva commented Jun 4, 2024

Hi @urugator @mweststrate I'm keen to hear you thoughts, I understand wrapping class component with function component is not a easy decision, but this is the only way to schedule a SyncLane update without hacking react internal logic (I have few solutions in this direction)

@jzhan-canva jzhan-canva changed the title Reintroduce observable state,props,context in mobx-react. solve tearing Solve tearing in class component,.Reintroduce observable state,props,context in mobx-react. Jun 4, 2024
@jzhan-canva jzhan-canva changed the title Solve tearing in class component,.Reintroduce observable state,props,context in mobx-react. Solve tearing in class component. Reintroduce observable state,props,context in mobx-react. Jun 4, 2024
@jzhan-canva jzhan-canva force-pushed the mobx-react-observable-props branch from 1840137 to 6ff48f3 Compare June 4, 2024 09:10
@jzhan-canva jzhan-canva changed the title Solve tearing in class component. Reintroduce observable state,props,context in mobx-react. Reintroduce observable state,props,context in mobx-react Jun 4, 2024
@jzhan-canva
Copy link
Contributor Author

jzhan-canva commented Jun 4, 2024

Close this PR as I need to reorganise the changes
reopened

@jzhan-canva jzhan-canva closed this Jun 4, 2024
@jzhan-canva jzhan-canva reopened this Jun 4, 2024
@mweststrate
Copy link
Member

Thanks for the extensive work here! I'll need to study more carefully the effects of this change, but I'm quite swamped atm. Feel free to ping if I didn't circle back in ~2 weeks!

unmount()
})

test(`Observable props/state/context workaround`, () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve this test.

@@ -1115,189 +1125,12 @@ test(`Observable changes in componenWillUnmount don't cause any warnings or erro
consoleWarnSpy.mockRestore()
})

test(`Observable prop workaround`, () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve this test.

container.querySelector("div")?.click()
})
expect(container).toHaveTextContent("1")
expect(renderCount).toBe(2)
Copy link
Collaborator

@urugator urugator Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be 1. The actual number isn't super relevant, it can depend on some React batching implementation details.
The point is that we must not issue an update on ANY component, when a component is already updating.
We use isUpdating flag to prevent updating self, but we must avoid calling forceUpdate also on other components - in this case on Child component, which is subscribed to Parent.props, because we leaked Parent.props to Child via callback.
It's like calling childRef.forceUpdate() during Parent's render. We must not do that. It's an oversight in the original implementation. Once component is updating (Parent recieved new props) the only proper way to propagate change to Child is via props. This would normally happen if you would create the callback during render.
The test was supposed to demonstrate that while this behavior is not correct, there is this unintended "feature" that you can send the same callback ref and still get the update (which is not supposed to happen with memo).
So to fix this, we probably just need to make isUpdating flag global.

Another way to put it is, that props must be observable for every derivation, except for observer derivation - that is any user defined computed/reaction/autorun.

Hope it's a bit clearer now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a bit less obvious example of usage that will break once it's fixed:

@observer
class Parent extends Component {
  store = observable({
    get cmp() {
      return this.props.foo + 5;
    }
  })
  render() {
    return <Child store={store} />
  }
}

@observer
class Child extends Component {  
  render() {
    return store.cmp;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@urugator I see what you mean now
yes this is an unintended "feature" that let child display "correct" value, therefore I feel removing it will cause unexpected issue in our fairly large codebase

that props must be observable for every derivation, except for observer derivation - that is any user defined computed/reaction/autorun.

This is something I can do, as per this commit, I can change globalState.trackingDerivation to be an array, and add isReactObserver(name TBD) to IDerivation

Eventually, during props' get method, I can check if other react observer is tracking it and show a error in console
image

@jzhan-canva jzhan-canva requested a review from urugator June 14, 2024 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants