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

Add fluxProvider.useEvalAsync() #67

Merged
merged 8 commits into from
Nov 7, 2016
Merged

Conversation

afiedler
Copy link
Contributor

@afiedler afiedler commented Nov 2, 2016

Adds an optional setting to fluxProvider to cause the flux service to wrap all $scope.$listenTo callbacks in $scope.$applyAsync.

This fixes #66, where in some cases the $listenTo callbacks were getting called outside of the digest cycle.

This is inspired by the useApplyAsync option in the $http service in Angular.

Wraps all calls to callbacks registered with $listenTo in $applyAsync
@Guria
Copy link

Guria commented Nov 2, 2016

Unfortunately this repo is unmantained now, since @christianalfoni not interested in it.
Now we actively support only cerebral-js which is view agnostic. But unfortunately have no angular implementations for cerebral v2 since we don't have much demands on it.

Feel free to start a fork or ask @christianalfoni to transfer repo maintainance.

@jrust
Copy link
Collaborator

jrust commented Nov 2, 2016

@Guria I'm still using it heavily and @christianalfoni gave me repo rights a while ago. If it'd be better to have repo transferred entirely let me know.

@jrust
Copy link
Collaborator

jrust commented Nov 2, 2016

@afiedler code looks good. Curious if you think true should be the default and if it appears to change performance at all?

@Guria
Copy link

Guria commented Nov 2, 2016

@jrust oh, sorry for misinformation. Glad that there is active maintainer here.

@jrust
Copy link
Collaborator

jrust commented Nov 2, 2016

No problem @Guria

@afiedler just saw your comment on the other issue on speed. Tried it out locally and the applyAsync was causing errors because it was batching the changes up until 10ms later which led to some timing issues. However, switching to evalAsync did clear up most of those and didn't exhibit any slowdown. There are still a few strange behaviors when dealing with animations, but that could be something strange we're doing. Does evalAsync work for you?

@afiedler
Copy link
Contributor Author

afiedler commented Nov 3, 2016

@jrust, yeah I just tested it with $evalAsync on my project and that works fine. I pushed two commits that make the change to this PR.

I'm not sure about the default. It seems like this is the "proper" way to do it. The issue is that baobab runs its callbacks in a window.setTimeout, which means they may or may not run as part of the digest cycle. So this fixes that problem. But I'm not sure if doing this would cause anyone any issues that depended on the previous behavior.

I guess I'll leave the decision on the default up to you 😄 . As long as I don't have to litter my code with $evalAsync, I'm a happy camper.

@afiedler afiedler changed the title Add fluxProvider.useApplyAsync() Add fluxProvider.useEvalAsync() Nov 3, 2016
@jrust
Copy link
Collaborator

jrust commented Nov 3, 2016

I agree. I think its saner and safer behavior to be default true. If you can make it true by default I'll merge it in and make a release

@afiedler
Copy link
Contributor Author

afiedler commented Nov 4, 2016

@jrust, I changed the default of useEvalAsync to true and updated the specs and README.

@jrust
Copy link
Collaborator

jrust commented Nov 7, 2016

Looks great, thanks for your work on this.

@jrust jrust closed this Nov 7, 2016
@jrust jrust reopened this Nov 7, 2016
@jrust jrust merged commit 3e26655 into christianalfoni:master Nov 7, 2016
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

Successfully merging this pull request may close these issues.

Some $listenTo callbacks run outside of the digest cycle when Baobao is in asynchronous mode
3 participants