mobxjs / mobx-react

React bindings for MobX
https://mobx.js.org/react-integration.html
MIT License
4.85k stars 350 forks source link

deepEqual condition for re-rendering? #906

Closed OskarKaminski closed 3 years ago

OskarKaminski commented 3 years ago

I guess the tradeoff of mobx-react performance is that it really does care only about the references not comparing the values?

Let's say we have an observable a = {foo: 'bar'}

Is there any way for mobx-render to not re-render components observing for a when we reassign {foo: 'bar'} (deeply equal) object to the observable a?

mweststrate commented 3 years ago

https://mobx.js.org/observable-state.html#available-annotations

On Sat, Oct 10, 2020 at 7:18 PM Oskar notifications@github.com wrote:

I guess the tradeoff of mobx-react performance is that it really does care only about the references not comparing the values?

And we have an observable a = {foo: 'bar'}

Is there any way for mobx-render to not re-render components listening for a when we reassign {foo: 'bar'} (deeply equal) object to the observable a?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/906, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBEBU2RG2NWLTUEQDR3SKCQQRANCNFSM4SLHB2PA .

OskarKaminski commented 3 years ago

For such a simple case I could use observable.struct, but my problem actually lays in arrays.

When I assign new array [{id: 1}, {id: 2}, {id: 3}] to the observed array [{id: 1}, {id: 3}], all elements' components are being re-rendered. Using shouldComponentUpdate with props.item.id !== nextProps.item.id would fix it, but PureComponent doesn't have this lifecycle method. image

mweststrate commented 3 years ago

Sounds like you are not using key prop, or are passing maybe something else causing renders. Check the react devtools to see why your children are rerendering or create a small sandbox as demo of your problem.

On Sat, 10 Oct 2020, 21:01 Oskar, notifications@github.com wrote:

For such a simple case I could use observable.struct, but my problem actually lays in arrays.

When I assign new array [{id: 1}, {id: 2}, {id: 3}] to the observable array [{id: 1}, {id: 3}], all elements' components are being re-rendered. Using shouldComponentUpdate with props.item.id !== nextProps.item.id would fix it, but PureComponent doesn't have this lifecycle method. [image: image] https://user-images.githubusercontent.com/7963279/95663984-17ce2680-0b44-11eb-9732-c911af205fb5.png

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/906#issuecomment-706603646, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBAHKT6NB2YUKL2NGSTSKC4RFANCNFSM4SLHB2PA .

mweststrate commented 3 years ago

Or if all children in your object are always new, than that is probably what you want to fix, rather than the rendering

On Sat, 10 Oct 2020, 21:13 Michel Weststrate, mweststrate@gmail.com wrote:

Sounds like you are not using key prop, or are passing maybe something else causing renders. Check the react devtools to see why your children are rerendering or create a small sandbox as demo of your problem.

On Sat, 10 Oct 2020, 21:01 Oskar, notifications@github.com wrote:

For such a simple case I could use observable.struct, but my problem actually lays in arrays.

When I assign new array [{id: 1}, {id: 2}, {id: 3}] to the observable array [{id: 1}, {id: 3}], all elements' components are being re-rendered. Using shouldComponentUpdate with props.item.id !== nextProps.item.id would fix it, but PureComponent doesn't have this lifecycle method. [image: image] https://user-images.githubusercontent.com/7963279/95663984-17ce2680-0b44-11eb-9732-c911af205fb5.png

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/906#issuecomment-706603646, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBAHKT6NB2YUKL2NGSTSKC4RFANCNFSM4SLHB2PA .

OskarKaminski commented 3 years ago

https://github.com/OskarKaminski/react-mobx-minimal-setup/tree/poc-deep-equal Please take a look at this PoC.

image

Using profiler:

  1. Select one item (so it becomes green)
  2. Click reload (so it replace the array with the same values + 1 element in the middle)
  3. Stop the profiler

You should see 4 elements, previously selected is still green, but the profiler shows 4 elements were rerendered (as in my previous comment)

mweststrate commented 3 years ago

Yes that is entirely working as expected if you recreate the same data over and over again. You can bail out of rendering if you use React.memo for example, but instead Id try to fix it at the root and merge your fresh data more smartly, better do the comparison once when receiving the data, than in every render.

On Sat, 10 Oct 2020, 21:27 Oskar, notifications@github.com wrote:

https://github.com/OskarKaminski/react-mobx-minimal-setup/tree/poc-deep-equal Please take a look at this PoC.

[image: image] https://user-images.githubusercontent.com/7963279/95664345-56b1ab80-0b47-11eb-9bcc-c1212b495fea.png

Using profiler:

  1. Select one item (so it becomes green)
  2. Click reload (so it replace the array with the same values + 1 element in the middle)
  3. Stop the profiler

You should see 4 elements, previously selected is still green, but the profiler shows 4 elements were rerendered (as in my previous comment)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/906#issuecomment-706606591, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBH4HGJLHJJW4XJ4OKLSKC7U5ANCNFSM4SLHB2PA .

OskarKaminski commented 3 years ago

That's what I thought. Even though this is a pretty common use case. I have users choosing technologies they know, which triggers an action to fetch all updates (dependent on technologies chosen). Updates are already sorted on the backend, items keep the same IDs so it's a real shame there is no way to not force re-render those that have the same "key".

Currently, for ~10 updates it takes 30ms to re-render. I'll optimize it with react-window, but I was searching for a way to optimize it as much as I can before.

(profiler from real app) image

OskarKaminski commented 3 years ago

I've implemented a naive script that mutates the first array to apply the differences with the other. Tests work well, the performance is way better, but I have an issue with sorting it (arr1.sort((a, b) => b.date - a.date)).

The arr1 doesn't seem to be impacted by .sort() method. Is it because arr1 is a proxy?

export default (arr1, arr2) => {
    const arr1Ids = arr1.map(el => el.id)
    const arr2Ids = arr2.map(el => el.id)
    const removedItemsIndexes = arr1.filter(el => !arr2Ids.includes(el.id)).map(el => arr1.indexOf(el))
    const newItems = arr2.filter(el => !arr1Ids.includes(el.id))

    removedItemsIndexes.map(index => {
        arr1.splice(index, 1)
    })
    arr1.push(...newItems)
    arr1.sort((a, b) => b.date - a.date)
}
OskarKaminski commented 3 years ago

I've migrated to Mobx 6, moved the sort function to a computed getter, replaced observable with makeObservable and it seems to work.