Feature: error messages and performance improvement for properties in the prototype chain

[tbosch]

In dirty_checking.js, DirtyCheckingRecord#set object:

Walk the protoype chain of the object to find the object where the property belongs to (in the sense of hasOwnProperty()). If the property could not be found, throw an Error. If it was found, use that found object for referencing the field.

Maybe solve #21 first.

[caitp]

Do we really want to throw if it's not found? It might be found "eventually", like once the context is updated.

I'll hack together a PR for this, because it should be very simple, but I'll look at merging a fix for #21 before merging it.

[tbosch]

Cool, thanks!
Yes, we should throw, but only if !(prop in object), not if object[prop] === undefined...

[caitp]

Throwing breaks very basic tests for example:

    it('should insert at the end of the list', function() {
      var obj = {};
      var a = detector.watch(obj, 'a', 'a'); // can't watch these due to the properties not existing...
      var b = detector.watch(obj, 'b', 'b');

      obj['a'] = obj['b'] = 1;
      expect(detector.collectChanges()).toEqualChanges(["a", "b"]);

      obj['a'] = obj['b'] = 2;
      a.remove();
      expect(detector.collectChanges()).toEqualChanges(["b"]);

      obj['a'] = obj['b'] = 3;
      b.remove();

      expect(detector.collectChanges()).toEqualChanges([]);
    });

The whole concept of additions and removals gets broken if we do it this way... I guess I'll look at #21 first to see if that fixes this

[tbosch]

We could change these tests, so that they contain the watched properties
with an undefined value initially, e.g.

var obj = {a: undefined, b: undefined};

On Wed, Mar 26, 2014 at 12:22 PM, Caitlin Potter
notifications@github.comwrote:

Throwing breaks very basic tests for example:

it('should insert at the end of the list', function() {
  var obj = {};
  var a = detector.watch(obj, 'a', 'a');
  var b = detector.watch(obj, 'b', 'b');

  obj['a'] = obj['b'] = 1;
  expect(detector.collectChanges()).toEqualChanges(["a", "b"]);

  obj['a'] = obj['b'] = 2;
  a.remove();
  expect(detector.collectChanges()).toEqualChanges(["b"]);

  obj['a'] = obj['b'] = 3;
  b.remove();

  expect(detector.collectChanges()).toEqualChanges([]);
});

The whole concept of additions and removals gets broken if we do it this
way... I guess I'll look at #21https://github.com/angular/watchtower.js/issues/21first to see if that fixes this

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-38727852
.

[caitp]

I don't think it really makes sense to do that, in the real world objects can and do change at runtime, even though there can be some performance penalties in some engines for it. Watching a property does not necessarily mean that the property is defined, it just means that you want to be alerted when it changes.

Suppose I'm requesting data from a remote location, and I use an ng-repeat to iterate over the returned collection. Should the ng-repeat's watch throw because the data isn't available yet? If so, why? I can't think of a good reason for that. Should ng-repeat be "special" so that it can handle collections which are yet to be defined? Are we ensuring that properties are defined before they actually are?

I'm also not really convinced that it makes sense to cache the object that does have the "own" property, because this is a form of magic which is not clear to the developer. In some cases it works in their favour, but in some cases it won't. I'm not too keen on it.

I think the performance benefits in most cases are probably negligible, and the amount of magic and developer inconvenience that gets added is probably not helpful. I can be convinced otherwise, but to me this seems like the wrong approach right now.

[tbosch]

One benefit would be to get errors for typos in expressions. But maybe you are right and it leads to more problems. Let's keep it as it is and focus on other things :-)