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

Computed fields do not trigger digest cycle #327

Closed
mightyguava opened this issue Apr 13, 2015 · 9 comments
Closed

Computed fields do not trigger digest cycle #327

mightyguava opened this issue Apr 13, 2015 · 9 comments

Comments

@mightyguava
Copy link

Newly computed values are not reflected in the scope when they are updated. Try modifying the inputs on the Plunkr. The computed "fullName" is always a change behind. (I'm on Chrome 41 OSX 10.10)

http://plnkr.co/edit/SHJA4RPdn03mRbvGFwcy?p=preview

I expected this to just work, but I guess the behavior makes sense since it's hard for js-data to figure out which scopes depend on the computed value to trigger $digest cycles for. Is there a better way to do this? I was hoping I could replace some angular filters.

Btw, loving this library a lot.

@mightyguava
Copy link
Author

Hmm... maybe js-data-angular can call rootScope.$applyAsync() after updates to computed fields?

@jmdobry
Copy link
Member

jmdobry commented Apr 15, 2015

Except $applyAsync wasn't added to Angular until 1.3.x.

The fields are re-computed in response to a $digest cycle being called on the $rootScope, but apparently the changes aren't being batched within those $digest cycles.

It seems like it could cause an infinite loop of I change it so $digest triggers computed property recomputation which triggers $digest cycles which triggers computed property recomputation which triggers etc. etc. etc.

@jmdobry
Copy link
Member

jmdobry commented Apr 15, 2015

I've been meaning to have Resources emit a "change" event when the observer code detects changes in an object. This would enable you to do something like:

app.controller('UserCtrl', function ($scope, $routeParams, User) {
  // this gives you finer control over performance/when $digest loops happen
  // rather than js-data-angular nuking your whole app with a bunch of $rootScope.$apply calls
  User.on('change', $scope.$apply);

  this.user = User.get($routeParams.id);
});

@mightyguava
Copy link
Author

I'd only trigger $digest if the computed property's result actually changed. If the $digest then causes a change in the computed property's result again, then I'd say that's an application bug, though js-data would definitely need to do something to break the loop and error. You could probably do so by checking whether after calling scope.$digest(), the dependencies of the computed values changed and keep a counter on how many times that happens? Does sound ugly though.

The change event does sound useful in general. For my use cases, I think I'd only care if computed fields changed. Will there be some sort of granularity to filter the results by? And, will it be possible to subscribe to the change event on the Resource level and as well on the resource instance level?

Also, it'd be nice if there was some config option that could enable calling $applyAsync by default on computed field changes, for Angular 1.3+ code.

@jmdobry
Copy link
Member

jmdobry commented Apr 15, 2015

Here's another problem, all the code for handling computed properties is in js-data, which is framework-agnostic and has no knowledge of Angular or $digest stuff.

This might help you:

In your plunker you have:

.controller('FooController', function ($scope, DS) {
    $scope.user = DS.inject('user', {
      id: 1,
      firstName: 'Billy',
      lastName: 'Joe'
    });
    DS.bindOne('user', 1, $scope, 'user');
  });

Try changing it to:

.controller('FooController', function ($scope, DS) {
    $scope.user = DS.inject('user', {
      id: 1,
      firstName: 'Billy',
      lastName: 'Joe'
    });
    DS.bindOne('user', 1, $scope, 'user', function () {
      $scope.user.DSCompute();
    });
  });

It's not the most obvious thing, but it works. In fact, I could probably have js-data-angular do this for you...that would solve this problem. :)

jmdobry added a commit that referenced this issue Apr 15, 2015
@mightyguava
Copy link
Author

Awesome, that should work. Thanks!

I think it'd be nice to also have a way to proxy bindOne like:

.controller('FooController', function ($scope, User) {
  User.find(1)
    .then(function (user) {
      myCtrl.complexModel.setUser(user);
      user.DSBind($scope);
    });
});

The example is kind of contrived yes. I'm just trying to show that a lot of the time the user won't be bound directly on $scope, but instead as a property of a much more complex working model. I think what I'm getting at sounds like making the expr optional.

Not an immediate need for it. I can ping back when it comes up. Still in the early stages of integrating js-data into our codebase.

@jmdobry
Copy link
Member

jmdobry commented Apr 15, 2015

If I understand your complex model question correctly, then this should already work for you:

// let's say the controllerAs is "FooCtrl"
.controller('FooController', function ($scope, User) {
  User.find(1);

  // "expr" can contain "." operators
  User.bindOne(1, $scope, 'FooCtrl.complexModel.user');
});

@mightyguava
Copy link
Author

Hmm... Yes that works. I think I mostly find it strange that User is putting a value on the $scope for me, instead of me putting it on the my model and then telling the $scope to bind it.

@mightyguava
Copy link
Author

Not a big deal. I'm happy with closing this out since the main issue has been resolved.

@jmdobry jmdobry closed this as completed Apr 15, 2015
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

2 participants