Undefined exception

[barbatus]

I think here this.observeHandle needs to be checked if it's defined already.
I get undefined exceptions sometimes.

[Urigo]

Interesting, can you show me a reproduction?
Also, it look simple to solve, would you want to pull request it to get the credit?

[barbatus]

Pls check console
http://ng-meteor.meteor.com/
Repo https://github.com/barbatus/ng-meteor

I think the bottleneck here (and partly the reason) is that $rootScope.$apply() runs every time new element added to the collection initially. It's not optimal by itself. More appropriate strategy would be 1) apply scope after all initial elements added to the collection 2) apply every next single element changed or added.

In the deployed example above,
right after reactiveFunc has been executed reactively second time here (i.e. it has limit dependency defined already) for the collection foo2 , at the exact moment new element may have been inserted into the collection foo1 which applies scope and causes all $watchs to run and invalidate current computations (via dependency.changed). Then Tracker.onInvalidate is being executed but foo2 still doesn't have an observer to stop.

[Urigo]

About your first point - when we worked on the last performance boost we did exactly that at the beginning but then ended up improving that so much we didn't needed that, but it still might make since - @netanelgilad thinking about re-adding that?

@barbatus do you think that plus checking if

this.observeHandle.stop();

exists before executing that is enough?

If so, can you add a pull request for that simple check? I want to release a new version today or tomorrow and want to include that.

[Urigo]

by the way, very nice reproduction!

[barbatus]

It's ok if it's 10 elements in a collection but if it's 100 elements it might be freezing to apply scope 100 times.
I would suggest another one method - start (good symmetrical addition to the stop) and a boolean option to the constructor which indicates observing state at the construction.
Thus, an optimized way to follow may be:

$scope.foo = $meteorCollection(function() {
   return Foo.find();
}, true, false /*don't observe*/);
$subscribe('foo').then(function() {
   // Start observing when subscription is ready
   $scope.foo.start();
})

BTW, this commit seems didn't get to the last releast, do you plan to add it to next one?

[stlouisweb]

Perhaps a meteor.defer() could be injected in the $rootScope.$apply() loop to make the function non-blocking. I haven't looked at the code, but meteor defer would essentially pop of the callstack each new $rootScope.$apply() for each doc in the collection, freeing up the app to do other things while applying scope.

[Urigo]

@barbatus that's interesting but I'm not sure there is a problem... have you actually encountered a freezing problem?
if you will look at the $watch function you can see we make it very optimized and we tested it with a lot of items.
If it's ok I would rather not add it to the API, but if it does I might reconsider.
About your commit, I'm not sure.... do you want to reopen that pull request?

@stlouisweb I'm not sure I understand.. maybe can you show an example in a branch, even a non working one

[barbatus]

@Urigo BTW, that $watch looks strange to me, why it's not?:

self.$watch(function() {
 return getValue(self)
}, function(newVal, oldVal) {
  if (newVal != oldVal) {
    self.$$trackerDeps[property].changed();
  }
});

[barbatus]

@Urigo Just checked once more, my bad, that pull request was added right after previous release, so should be in a new one

[Urigo]

I'm not sure I understand your two last comments, sorry :)

[barbatus]

@Urigo

1. I mean this $watch. It's executed every $apply, hence, it needs if (newVal != oldVal)

2. I was wrong about the commit, never mind. (that PR will be in next release).

[netanelgilad]

@barbatus about 1.
The check is actually needed but it should not affect performance since newVal === oldVal only happens once in the watcher initialization as described in the angular docs https://docs.angularjs.org/api/ng/type/$rootScope.Scope#$watch.

[Urigo]

@netanelgilad can you add that now before I release a new version?