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

Make angular.$apply optional #299

Closed
miguelangelmunoz opened this issue Jul 21, 2016 · 14 comments
Closed

Make angular.$apply optional #299

miguelangelmunoz opened this issue Jul 21, 2016 · 14 comments

Comments

@miguelangelmunoz
Copy link

miguelangelmunoz commented Jul 21, 2016

In order to avoid calling scope.$apply(scope.infiniteScroll); every time, let's add the option to handle it with a parameter.

@graingert
Copy link
Collaborator

@miguelangelmunoz ngInfiniteScroll doesn't call $rootScope.$apply

@miguelangelmunoz
Copy link
Author

Sorry, scope.$apply(scope.infiniteScroll);

@graingert
Copy link
Collaborator

graingert commented Jul 21, 2016

@danielkocsan
Copy link

Hi @graingert,

The intention is to have an option to manage when angular digest is executed. This is for performance purposes.
In our application we use this library having tons of bindings so a digest flow can take long. Would be nice to have the possibility to execute digest wherever and whenever we want.

Miguel created a brach adding a new parameter where you can set avoiding the explicit call on $apply().

@graingert
Copy link
Collaborator

@danielkocsan
Copy link

Hi @graingert,

Not yet, it looks like a thing to consider for our further improvements. Thanks for the tip and also for the quick responses.

Currently though, we use this library and we are having performance issues which should be fixed.

Do you think we can apply this change? Could be useful for other users as well and doesn't affect the rest as it defaults to the current behaviour. @miguelangelmunoz has added test coverage too.

@graingert
Copy link
Collaborator

@danielkocsan I've not seen anything yet...?

miguelangelmunoz pushed a commit to miguelangelmunoz/ngInfiniteScroll that referenced this issue Jul 22, 2016
@miguelangelmunoz
Copy link
Author

@graingert you can find the Pull Request in the following link:

#300

@graingert
Copy link
Collaborator

I don't think it's a good idea to expose this sort of interface. I think this should always call $apply().

Also IMHOP ngInfiniteScroll is in permanent feature freeze / maintenance mode and people should be using https://material.angularjs.org/latest/demo/virtualRepeat or waiting for angular/components#823

@danielkocsan
Copy link

It would make sense to explain why do you think things rather then just saying them.

@graingert
Copy link
Collaborator

This is because handler is responding to a callback from the dom, like $timeout and $http this must invoke $apply for angular to update the model.

@danielkocsan
Copy link

This is not the same case.
The responsibility of this library is just to call the "loadMore" function whenever the user scrolls out of the container.
All managing your items and rendering them is (should be) out of scope.

@graingert
Copy link
Collaborator

the loadMore function is passed as a directive callback, all directive callbacks should trigger a digest cycle, eg ngClick

@danielkocsan
Copy link

You are safe if you do that but not efficient. That's why there are multiple performance issues reported with Angular 1.x

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

No branches or pull requests

3 participants