Potential usage of reserved keywords in Parser.getterFn

[zenorbi]

At the line 928:
: '((l&&l.hasOwnProperty("' + key + '"))?l:s)') + '.' + key + ';\n';

should be replaced to
: '((l&&l.hasOwnProperty("' + key + '"))?l:s)') + '["' + key + '"];\n';

as the key can potential be a reserved keyword like float, class, for. I know about 1 instance where the key is "in" which is part of the for (k in o) syntax.

[gkalpak]

I don't think there is a problem with accessing a property using the dot notation even if that property's name is a reserved keyword (but I might be missing something).

@zenorbi, do you have an example expression that causes an error or unexpected behaviour due to this ?

[zenorbi]

In IE8 it does throw an exception. I know that IE8 doesn't participate on automated testing, but this is only a 4 character modification.

[Narretz]

We don't support angular in 1.3.x. Does it happen in 1.2.x?

[zenorbi]

It doesn't happen there as the key is wrapped in square brackets.

[gkalpak]

@zenorbi: Could you post the breaking example. I am just curious and I wasn't able to reproduce it.

[zenorbi]

I've been able to narrow it down to this:
<div ng-class="{in: animate}"></div>

A div like this gets inserted by angular-ui and causes the parser to try and parse the "in" part.

[btford]

@zenorbi if you have a reproduction for Angular 1.2.x that is broken in IE8, we can take a look at it.

[zenorbi]

@btford Angular 1.2.x doesn't have this bug, as that uses square brackets. This is the line from angular 1.2.25:
: '((k&&k.hasOwnProperty("' + key + '"))?k:s)') + '["' + key + '"]' + ';\n' +
which is correct, as this will translate to (...)["in"] instead of what 1.3 would do (...).in.

[caitp]

that sounds like a pretty simple fix then, did we change that for perf
reasons?

On Wed, Sep 17, 2014 at 1:59 PM, Zenorbi notifications@github.com wrote:

@btford https://github.com/btford Angular 1.2.x doesn't have this bug,
as that uses square brackets. This is the line from angular 1.2.25:
: '((k&&k.hasOwnProperty("' + key + '"))?k:s)') + '["' + key + '"]' +
';\n' +
which is correct, as this will translate to (...)["in"] instead of what
1.3 would do (...).in.

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

[zenorbi]

I know that most of the stuff doesn't actually work in IE8 like Object.getPrototypeOf or Object.create but I was able to shim that in along with a bunch of other stuff. Currently this is the only thing which needs modification on the angular.js file. I can just keep replacing this line in each angular version, I'm just hoping that because this is such a small adjustment, it can live in the real source.

[caitp]

if we had a good reason to change it, then it probably can't. But lets find out. If you want you could submit a fix for it, it should be a pretty trivial PR

[lgalfaso]

@caitp it was done for performance reasons c54228f

[zenorbi]

@lgalfaso Ok then, it was worth a shot anyway. Thank you.

[caitp]

I don't believe the performance reasons are really correct in this case, because it breaks a lot of valid expressions, and makes the whole [] syntax completely meaningless in our parser. I would personally vote to revert those, as it's unlikely to make a significant difference.

[tbosch]

For IE9 we should actually check the value of key. If it is a reserved word we should use the old [], otherwise the . syntax.

[caitp]

.property is also broken for anything containing whitespace, or anything containing special characters like + or @ or similar

[caitp]

Igor says we use a different path when [] notation is taken, so the non-ident characters / whitespace issue is moot I guess. I guess testing for reserved words makes sense however

[lgalfaso]

I also think we should use the [] syntax if key is a reserved word and the . syntax otherwise (I think that the number of times people will run into this is so minimal that it is better to just to it for all browsers)

[lgalfaso]

reading at the code, I am not sure how just <div ng-class="{in: animate}"></div> would end up running into the issue described...

[caitp]

the object literal parser doesn't look like it should run into that, this is true

[zenorbi]

I've found that this also crashes Android 2.x browsers as the parser does throw SyntaxError.

[lgalfaso]

@zenorbi would it be possible to have a minimal example where this is a problem?

[zenorbi]

Of course, you just need to run an ng-app with this element inside:
<div ng-class="{in: animate}"></div>
The ng-class will parse the in part as well which will produce a Function at the end. Because of the in the Function() call will fail.
Here is a plunker for it: http://plnkr.co/edit/G21T47H3R4lBZG4mSC9Y

[lgalfaso]

Ok, I am able to reproduce this. I think that the proposed patch is not the right one, will post a patch in a few

[lgalfaso]

The issue is that we were generating getter functions for things that needed no getter function. These were for filters and object properties. #9229 should take care of this
@zenorbi it would be great if you can try the patch