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

Is ChangeDetectionStrategy.OnPush required? #31

Closed
westonganger opened this issue Apr 12, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@westonganger
Copy link
Contributor

commented Apr 12, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

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

@Jonatthu

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

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

@returnlytom

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2017

@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

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

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

@Jonatthu

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

@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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.