fix(ngRepeat): support complex assignable aliasAs expressions

[caitp]

Parse aliasAs as an expression, and assert that the expression is assignable.

Just throwing together a quick fix, but I'm not sure this is what we want to do, especially since we
want to change the behaviour of aliasAs to provide more data. /cc @matsko / @shahata / @petebacondarwin

BREAKING CHANGE

Previously, any name passed as an expression would make up a single property
name, including constant values such as 1, NaN, null, undefined, or even
expressions such as function calls or boolean expressions.

Now, more complex expressions are possible, allowing the collection alias to
be assigned as a property of an object --- however, if the expression is not
determined to be assignable, it will throw.

Fixes #8438

[petebacondarwin]

spurious = sign at the end of this line ?

[petebacondarwin]

Other than the error docs changes I think this looks good.
Regarding the breaking changes commit message, I think that we can offer a workaround for these "non-assignable" properties such as, NaN, 1, null, etc:

<div ng-repeat="item in items | filter:x as this['NaN'] track by $index">{{item}}</div>

[shahata]

I'm not 100% sure that it is a good idea to fix this. I mean, I'd like to think of this alias as an intermediate variable that does not exist outside of the scope of ng-repeat. I might be wrong, but putting it on a controller seems like bad practice and similar to stuff like assignment operator in expressions, which we are attempting to move away from. Moreover, #8398 would make this impossible since we are probably going to create a separate alias instance for each child scope.

[caitp]

#8398 wouldn't make it impossible, it would just mean the value assigned and scope of assignment would be different. This CL wouldn't cause any issues with that.

I'm also not sure it's a problem to assign it to a controller, or a nested object --- but folks opinions are appreciated.

[caitp]

Also, this has nothing whatsoever to do with assignment operator in expressions, so it's really not clear what you're even talking about there =P

[petebacondarwin]

I believe the primary use case is to assign it to a Controller that was created with controller as

[caitp]

You wouldn't be able to assign it to the controller unless it was created with "controller as", since it assigns in the context of scope...

[petebacondarwin]

@caitp - what @shahata is saying is that we are trying to get away from writing to the scope in templates (i.e. the assignment operator in expressions: ng-click="foo = 'bar'") and that being able to assign the results of the filter in ngRepeat has potential to affect things outside the scope of the repeater

[caitp]

@petebacondarwin the thing is, that's irrelevant here --- such an expression would throw a minErr (due to not being assignable) --- yes, we can assign properties to scope, but we're doing that anyways --- the only difference is that this means we can assign properties to objects in scope rather than just to scope directly. That point seems to be missed --- this is no different from just writing to $scope[aliasOf], except that it supports nested objects --- there are no real side effects here, because we specifically require an assignable expression --- no side affects that didn't exist originally (like the ability to overwrite some value)

[shahata]

The alias must be stored on some scope (preferably on the child scopes imo), that's just how things work and there's not a lot we can do about that. But, I think allowing to put the alias in a nested object or on the controller will only encourage people to access it from their JS code, which seems just wrong. The thing is I don't really see the advantage of allowing this, maybe I'm missing something - When do you think someone might have a good reason to put the alias on some nested object?

[petebacondarwin]

@caitp - You are right that you can use alias as to write to a property on the containing scope, but with this change you could potentially write to a property on any parent scope. Admittedly this is a strange thing to do and I guess that given we have accepted that the containing scope can be polluted by alias as then we shouldn't worry too much.

@shahata - the use case is controller as. See #8438. People who use this construct don't like to have to reference $scope in their JS code.

[petebacondarwin]

I agree it is a bit messy and I wouldn't do it personally. In fact there are very few scenarios where I would use alias as at all, since I would always prefer to set up the filtering in controllers in the first place. But this is what was agreed since it can save a lot of boilerplate $watches in JS when you have nested filtered repeaters

[caitp]

will only encourage people to access it from their JS code, which seems just wrong.

How does this seem wrong? We don't care if people access it from their JS code, it does not matter

[caitp]

Literally the only difference this makes is that,

Instead of alias as foo['bar'] creating $scope["foo['bar']"], we instead create $scope["foo"]["bar"].

Instead of creating $scope["foo()"], we throw. Instead of creating $scope["foo = []"], we throw. This is a good thing.

[caitp]

None of this does "more" to "encourage people to use these collections from JS" --- the collections were exposed to JS already. None of this encourages people to do things they shouldn't, it just gives a minor convenience and prevents people from doing things they don't expect it to do. The argument that this is harming users is completely bogus to me, and we need to do better than that to avoid this, IMO.

[caitp]

For what it's worth, I do agree that the alias should go into child scopes rather than the containing scope, that would be ideal. We could also blacklist the expression if it contains $parent, if that would make you feel better --- but that complicates code somewhat.

However, I don't think we should stop people from putting the value where they expect to put it. If you're trying to assign the value to Foo.bar, it had better go to ['Foo']['bar'] and not ['Foo.bar']

[shahata]

I agree 100% it should go into child scopes instead, but then we don't need this PR imo. The alias won't be any different from the single array item that is put on the child scope. Then we will also not need to worry about $parent (which is not a big deal anyway) since as you said, it would write to childScope['$parent.something']

[caitp]

I agree 100% it should go into child scopes instead, but then we don't need this PR imo

This is what I disagree with --- It's unexpected that $parent.foo assigns to $scope["$parent.foo"] --- this is not what people are expecting, and is not the behaviour you typically get with expressions. This is inconsistent and bogus.

If you want to prevent people from using . or [/] at all in their alias expressions, then lets change the regexp to fix that. But leaving it as it is is not good enough, because it creates confusion. The language is simple, lets not add all these corner cases.

[caitp]

Yes, that's why y'all are CC'd on this bug, to figure out the disaster points =)

[shahata]

But if we have something like item in items | filter:x as ctrl.alias and the alias is actually an object with $collection, $index, etc. - won't childScope1.ctrl.aliasand childScope2.ctrl.alias be the same instance?

[shahata]

I mean we would like childScope1.ctrl.alias.$index to be 0 and childScope2.ctrl.alias.$index to be 1...

[caitp]

not if we assign an empty object to the first key of the expression

[shahata]

yes, but then you won't be able to access anything in your controller inside the ng-repeat...

[caitp]

so what?

[shahata]

If I have ng-click="myCtrl.handleClick()" inside the ngRepeat it won't work...

[caitp]

and... so what! you aren't obligated to put the alias there. We could also just always call it $collection in child scopes, regardless of the alias

[shahata]

I don't think anyone who doesn't really understand the internals of angular and prototypical inheritance would expect this to happen, and I don't believe there is anyone who would actually WANT this to happen.

[petebacondarwin]

We should definitely not be adding the filtered items alias in the childscopes.

[caitp]

@petebacondarwin the main reason it was asked for in the child scopes is so that a context for $index and etc would be added to an aliased property, to make nested repeaters easier to deal with (parentAlias.$index vs childAlias.$index)

However, there are ways around this with the (key, value) syntax anyways, so it might not be super worthwhile

[shahata]

@petebacondarwin - see #8398

[petebacondarwin]

As far as this PR goes, I think it is OK. I think we could merge it.
The original motivation was to enable filtered repeaters in templates without having to resort to lots of JS. This PR tries to support connecting these items to object that are only defined in JS, so it is kind of anathema but not the end of the world. Going further than this seems like a step too far.

[petebacondarwin]

@shahata - I put my comments on that issue already

[joshbowdoin]

Definitely need the alias on the parent scope, as that enables you to do things like show / hide an "add item" button if no items, etc. For me, that is the only place it is really needed.

As far as putting it on the controller as variable - and that making it available to the JS... the $scope variables are also available to JS - there is no difference here.

The reason for the request was that myCtrl.alias is much more readable to me. I know that this variable is originating somewhere within myCtrl's scope. I don't have to spend time trying to determine that. The request has nothing to do with accessing/not accessing it from the controller's JS.

[shahata]

Why not use toThrowMinErr?

[IgorMinar]

+1

[caitp]

Because $exceptionHandler causes problems for this, and it would make the test a lot bigger to reconfigure $exceptionHandler to rethrow instead

[caitp]

I can do it, but jus'sayin, other tests in this suite are using the same strategy here because of this

[shahata]

Another important reason this is needed is when you want to put the alias in a higher level scope as described in e0adb9c#commitcomment-7254841 (much more convincing then controllerAs )

[IgorMinar]

ngRepeat's as is more aligned with ngController's as which doesn't support assignable expressions (and it really shouldn't otherwise we could easily leak memory by referencing controllers from long-lived scopes).

As far as ngRepeat goes assigning filtered collection to upper scopes could leak to similar leaks. Assigning the result to controller instances is also a bit weird use-case. ngModel is special because of two-way data-binding.

How about instead of checking if the alias is assignable we check if it matches a regular expression for a valid identifier? that way we provide good error reporting (that's what triggered this PR in the first place) and keep the door open for making this assignable in the future after we observe the real-world usage and find that making the expression assignable would outweight all the concerns about memory-leaks and spaghetti code that have been raised in this discussion?

btw I don't like the idea of conflating the alias with a context for $index and friends. see my comment here: #8398 (comment)

[caitp]

okay, we could make the regexp more strict, but I expect we'd run into issues in the future (as we do for other identifiers) due to people wanting to use non-latin characters

[IgorMinar]

Using the assignable strategy would not help to deal with the non-ident
chars. Just saying :-)
On Aug 13, 2014 5:47 PM, "Caitlin Potter" notifications@github.com wrote:

okay, we could make the regexp more strict, but I expect we'd run into
issues in the future (as we do for other identifiers) due to people wanting
to use non-latin characters

—
Reply to this email directly or view it on GitHub
#8440 (comment).

[caitp]

I've changed this in order to make it a bit more strict and help prevent people from doing weird things

[gabrielmaldi]

I'd like to be able to assign the results to a property inside an object that uses two-way data binding in a directive:

angular.module("directives").directive("myDirective", function () {
    return {
        scope: {
            myObject: "="
        },
        templateUrl: "myDirective.html"
    };
});

<input type="text" ng-model="searchText"><input>
<div ng-repeat="item in items | filter:searchText as myObject.filteredItems"><div>

So that the client of this directive has access to the list of filtered items through myObject.

But the current implementation of aliasAs is too restrictive to allow this scenario.

[caitp]

You can't, Igor didn't like this because it would let people move alias'd collections into places where they'd be kept alive outside of the lifetime of the ngRepeat --- and thus be potentially leaky. Sorry :( (not that you can't technically do this anyways, but it's not a feature of ngRepeat, basically) --- the original implementation of this PR would have let you do that, but it was changed to limit you to simple identifiers rather than things which look like they'd be properties of other objects --- so that you don't accidentally create properties like "foo.bar" or whatever.

[gabrielmaldi]

I know, I read the whole thread 😃 I just thought I'd share my real-world use case since Igor said:

and keep the door open for making this assignable in the future after we observe the real-world usage and find that making the expression assignable

Thank you for your quick answer!

EDIT:

Would it be too counter intuitive to allow aliasAs to be a function? This way ng-repeat could call it with the results as an argument, allowing you to do whatever you like with them.