mobxjs / mobx-angular

The MobX connector for Angular.
MIT License
483 stars 59 forks source link

Is ChangeDetectionStrategy.OnPush required? #31

Closed westonganger closed 7 years ago

westonganger commented 7 years ago

Is ChangeDetectionStrategy.OnPush required or can I leave it off.

If I can leave it off, What are the pros/cons to turning it on or leaving it off?

Jonatthu commented 7 years ago

With OnPush you can have the best performance in change detection and when you are doing heavy loops in each event for example mouse hover an item fires a lot of events and for each event I have a for loop doing a logic behind scenes when I was doing without onpush the app was very unresponsive after 30 hover events

https://www.mapwords.com/home/(search:sushi/33.6586518/-117.75491799999999/5/0/0/false/1)

Hover the results and you will know what I am talking about, this was made with mobx and angular 4 and this plugin :)

westonganger commented 7 years ago

If its so much better why is it not always enabled?

Jonatthu commented 7 years ago

Because some components are not using *mobxAutorun for example a component that will be shared in a lot of containers||components like a card and you just need to pass the data through the @Input() decorator, for example in that link in the results I'm using a card component passing data trough the @Input() decorator and just managing the mouse hover event outside the component with mobx

In other words, it depends of your goals an design architecture of each component

westonganger commented 7 years ago

I feel like it would be helpful to have an explanation for this in the Readme.

returnlytom commented 7 years ago

@Jonatthu Thanks for taking the time to explain, found it very helpful. Also, mapwords looks great, nice work!

To supplement the explanation, perhaps we can create an example benchmark app to determine the performance implications of the different change detection strategies.

Jonatthu commented 7 years ago

@westonganger I'm going to pull request a better documentation of this @returnlytom Thanks and yes I will see what I can do, I was doing this with google chrome profiling tools and I notice a big improvement.

The simple answer is that Angular is not checking and updating the view until the result is ready, that is happening with OnPush and the directive *mobxAutorun is deciding when is that true.

https://vsavkin.com/change-detection-in-angular-2-4f216b855d4c

westonganger commented 7 years ago

Also can I set changeDetection application wide or do I have to specify each component?

Jonatthu commented 7 years ago

You can do it from the root but basically that means all your components will depend from the store and will use *mobxAutorun, unless you specify on a component that you strategy is the default. From my personal experience I prefer to put it on default and just specify OnPush where I need it. If you app is small I think you can be free of make by default OnPush or if you are using angular cli you can put on the configs that your default change strategy is on push

adamkleingit commented 7 years ago

@westonganger If you stick to relying only on @Inputs and stores with *mobxAutorun then you can use OnPush everywhere. You can set it in the .angular-cli.json file:

  "defaults": {
    "component": {
      "changeDetection": "OnPush"
    }
  }

On the other hand, if you are building a simple app I wouldn't do premature optimization.

@Jonatthu Thanks, and looking forward to the README PR :)