fix($parse): allow assignment on objects in locals

[jbedard]

Fixes #4664

I saw this issue on the 1.4 list but the fix seems simple...

[googlebot]

CLAs look good, thanks!

[gkalpak]

If this lands, it would be good to update the docs as well.

BTW, the whole concept of locals in a setter makes no sense to me. Anyone can think of a use-case where this is useful ?

[lgalfaso]

I have to admit that I did not read the patch with a lot of care, but it looks like there are cases that this PR is not handling Eg foo.bar[shell]

[jbedard]

I think that specific case already works. Should probably add more tests similar to that though...

[jbedard]

@gkalpak I agree it is a little weird but I think it should be consistent. Think of $parse('a.value = 1 && a.value')({}, {a: {}}), it really should return 1...

[jbedard]

I've updated the tests to better cover the 3 different assign functions: a =, (exp).c =, exp[exp] = (property, object field, object index)

[gkalpak]

Think of $parse('a.value = 1 && a.value')({}, {a: {}}), it really should return 1...

@jbedard, you think you mean:

var o = {}, l = {a: {}};
$parse('(a.value = 1) && a.value')(o, l);
expect(o.a.value).toBe(1);

which does already work.

After this PR, the above snippet will result in o -> {}, l -> {a: {value: 1}}, which is not that obvious (imo).

Do I miss something ?

[jbedard]

@gkalpak you're right it needs the brackets. The issue is that the assignment went to o.a while the read came from l.a. Doesn't that look wrong? a.value in two places in the same expression refer to different objects...

[gkalpak]

Doesn't that look wrong?

Not really 😄
The way I understand it, locals is a "helper" object that provides some values (i.e. overrides scope's values). But I find it unintuitive to assign values to it (but maybe it's just me).

If @IgorMinar thought this should be added, then it probably should :)

[jbedard]

(a.value = 1) && a.value returning undefined doesn't look wrong? 😳

Note that we are never assigning to locals but are fetching the a object from locals. The way you understand it is 100% correct, locals is a helper that provides some values, in this example that value is the a object...

[caitp]

missing: documentation of the extra parameter to Getter#assign() --- also missing stuff for the changelog. It shouldn't be a breaking change, but it needs to be logged

[caitp]

It is adding some code for something that I don't really expect anyone to actually use, though

[jbedard]

assign() already has that param (https://github.com/angular/angular.js/blob/master/src/ng/parse.js#L664, https://github.com/angular/angular.js/blob/master/src/ng/parse.js#L688). It is just missing from the getterFn.assign case.

I agree it isn't really useful, or at least I've never wanted to modify an object in locals. I think it is logically correct though: (a.value = 1) && a.value should return 1 whether a is a local or not...

[caitp]

this is what I'm referring to, @jbedard

[jbedard]

This is just 1 of 3 assign implementations though. The others already had the third param so it is nothing new, it was just missing from this implementation...

You want to document it to make it public though?

[caitp]

there's no reason not to document it --- nobody has a good reason to use it anyways, but we aren't using it as a private API anywhere

[jbedard]

It's used within this file, just this version (unlike the other 2/3) did not support it...

[lgalfaso]

Hi @jbedard, sorry to insist on this same point once again, but the patch as-is is not handling many cases. Here are a few

expect(function() {
  $rootScope.$eval('{c: null}.c.d = 1');
}).toThrowMinErr(/* */);

delete $rootScope.x;
$rootScope.$eval('x[a][b] = true', {a: 'foo', b: 'bar'});
expect($rootScope.x.foo.bar).toBe(true);

delete $rootScope.x;
$parse('x[a][b]').assign($rootScope, true, {a: 'foo', b: 'bar'});
expect($rootScope.x.foo.bar).toBe(true);

delete $rootScope.x;
$parse('x[a].bar').assign($rootScope, true, {a: 'foo'});
expect($rootScope.x.foo.bar).toBe(true);

[lgalfaso]

just like these, there are several cases than need new tests

[jbedard]

@lgalfaso sorry I guess didn't understand your original point, thanks for clarifying!

I think those are all from the locals object not being passed through to sub expressions. Probably just need to add , locals in a few places. However this suddenly this looks bigger then just patching that one assign method :P

[lgalfaso]

@jbedard yep, it is a bigger patch than what it looks like... that is why it was a 1.4 candidate :)

[jbedard]

I still think it won't be too bad, but I'll try it later and report back. Thanks for the test cases!

[lgalfaso]

please, do not get me wrong, I think it is possible but it is a patch that needs some careful work

[jbedard]

@lgalfaso for the $rootScope.$eval('{c: null}.c.d = 1') case - how is that related to locals? It is debatable what that one should do...

The other cases are fixed. Those were introduced in c03ad24 which did not pass the locals through (unlike the non object creating case that has always passed it through).

[petebacondarwin]

why not pass the locals in here too?

[jbedard]

Because we are specifically assigning to a property on the object o which was already fetched from the scope/locals. For example with (object).b = 1... (object) is evaluated using scope+locals, but the .b = 1 should only be evaluated on the object returned from (object).

[petebacondarwin]

After this PR, the above snippet will result in o -> {}, l -> {a: {value: 1}}, which is not that obvious (imo).
Do I miss something ?

This seems reasonable to me - if we are providing an object in the locals that overrides an object on the scope then both reads and writes should go to that locals object. For me the analogy with JavaScript would be...

var obj = { name: 'outer' };
function changeObj() {
  var obj = { name: 'inner' };
  expect(obj.name).toEqual('inner');
  obj.value = 42;
  expect(obj.value).toEqual(42);
}
changeObj();
expect(obj.name).toEqual('outer');
expect(obj.value).toBeUndefined();

[jbedard]

Updated and rebased.

[petebacondarwin]

Landed as 8690081
Thanks @jbedard et al