mobxjs / mobx-react

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

How to efficiently use observables with callbacks #845

Closed lukasbash closed 4 years ago

lukasbash commented 4 years ago

For a react web application I am using MobX and a third party library (this one precisely). Now there are several components that offer callbacks to render custom content. Let me make a concrete example.

The List component offers a callback to render an item. The structure looks like this:

@observer
export class MyList ... {
  @observable private _selectedItem: number | undefined;

  private _onRenderItem = item => {
    // check if the item is the selected one
    const isSelected = this._selectedItem === item.id;

    // if it is the selected one, I want to e.g. apply some style
    return <MyListItem isSelected={isSelected} />
  }

  render() {
    return <List onRenderItem={this._onRenderItem} /> // ofc I pass more than just the function, just probably not worth to mention it here
  }
}

As in this example, the List component is from the third party library, it does not know to re-render this item (i.e. calling the render function for the item that changed). Correct until here right? There is no way that MobX can track the change trough a component that is not an observer, right? So with this implementation the callback breaks the observable chain.

Now, of course I could alter the code to wrap the callback into an observer like this:

@observer
export class MyList ... {
  @observable private _selectedItem: number | undefined;

  private _onRenderItem = item => {
    return (
      <Observer>
        {() => {
          // check if the item is the selected one
          const isSelected = this._selectedItem === item.id;

          // if it is the selected one, I want to e.g. apply some style
          return <MyListItem isSelected={isSelected} />
        }}  
      </Observer>
    );
  }

  render() {
    return <List onRenderItem={this._onRenderItem} /> // ofc I pass more than just the function, just probably not worth to mention it here
  }
}

But here another problem comes up: When using this approach I can see all items re-rendering (using the MobX devtools in chrome) and not only the items where the isSelected prop changed.

How can I avoid this re-rendering? Or do I have a wrong approach in the first place?

P.S.: I cannot (and do not want to) re-implement the class of the third party library, so I would like to stay with the usage of the available callbacks.

danielkcz commented 4 years ago

Can you verify that MyList is not-rendered on selection change? If it does it means that List will re-run all callbacks.

lukasbash commented 4 years ago

@FredyC Well, in both cases MyList does not re-render. But this is fine, isn't it? Because MobX should trigger the re-renders as precise as possible, and the key of the selected item is not used in the parent components themselves. If MyList would re-render, then again as you say, List would re-render all items (by invoking the callbacks).

I expect that - if the selection changes - at maximum two items (of class MyListItem) re-render and nothing else.

mweststrate commented 4 years ago

this_selectedItem changes, and that is used by all rows, so all rows will have to re-evaluate. The simple solution is to extract a component, and give it a computed property so that the output of the is-selected state can be cached. Or you can change the above to const isSelected = computed(() => this._selectedItem === item.id).get() so that you create an inline computed / cached expression

On Mon, Mar 9, 2020 at 2:01 PM lukasbash notifications@github.com wrote:

@FredyC https://github.com/FredyC Well, in both cases MyList does not re-render. But this is fine, isn't it? Because MobX should trigger the re-renders as precise as possible. If MyList would re-render, then again as you say, List would re-render all items (by invoking the callbacks).

I expect that - if the selection changes - at maximum two items re-render.

— 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/845?email_source=notifications&email_token=AAN4NBH5RUGCMK2TUFFKXUDRGTZDXA5CNFSM4LEI4I62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOHH7XY#issuecomment-596541407, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBEPNWKUZKK6DAJNPUTRGTZDXANCNFSM4LEI4I6Q .

lukasbash commented 4 years ago

@mweststrate Oh damn! I did not think of a solution like that. I tried the inline computed and it works like a charm. Thank you so much for the fast answers guys!

lock[bot] commented 4 years ago

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.

lock[bot] commented 4 years ago

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.