WIP: detect accidental interpolation

[IgorMinar]

throw an error when developer accidentaly uses interpolation instead of an expression

TODO:

• try to apply this to other directives
• try to apply this genericaly for directives with isolate scope definition
• add an error page

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#8262)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[IgorMinar]

Just an idea... do you guys like this? Can it be done in a better way?

[petebacondarwin]

Could it be applied automatically to directives that use isolated scope. I.E. In the following scenario:

scope: {
  a: '@',
  b: '=',
  c: '&'
}

the compiler would throw if b or c contained interpolation.

[caitp]

mentioned in the TODO already :) I don't think it's too bad, but it halt compilation? It would be more helpful if people could get a full dump of all of the cases in their app where this happens, rather than dealing with one at a time.

..and then there's the issue with arbitrary {{ testing, which we probably don't want.

[caitp]

Another thing we could do is perform this assertion in $parse() instead, so that we know it's always applied when an expression is parsed

[lgalfaso]

I like the idea, but there is an issue when the user changes the interpolation start symbol

[IgorMinar]

@lgalfaso that can be fixed

[IgorMinar]

@petebacondarwin that's on the todo list

[IgorMinar]

@caitp I like the idea of integrating this with $parse

[lgalfaso]

@IgorMinar what I am aiming at is that {{ cannot show up in expressions or JSON, but that does not mean that a user can change it to something that is valid. e.g. [[

[caitp]

phase 2: create a blacklist of start/end tokens which would be valid to the expression parser, and prevent people from using them

[btford]

I like it. Could belong in angular-hint, but this seems cheap enough to be worth adding to core.

/cc @caguillen214 @ealtenho

[rodyhaddad]

I explored this more with Igor, and decided not to make changes to $parse. Mainly because then it would have to dependant on $interpolate, creating a circular dependency which requires a hack to get around. And all that will do is create a better error message. So doesn't seem worth it.

The real problem is that ngBindHtml doesn't throw an error when an interpolation is used in its expression.

There are 5 directives in total that ask for expressions but don't complain when there's interpolation in them. The first 4 aren't worth the fix/refactor imho:

• ngOptions: You can interpolate whatever's in a ng-options attribute and it would still work. I doubt that it's a real issue, because ngOptions has its own micro-syntax, and you get an error if the resulting attribute value is incorrect.
• ngPluralize: You can interpolate ng-pluralize, even if it expects an expression that results in an object. This isn't that bad, as interpolating the values of ng-pluralize might be seen as a feature :)
• ngChange: You can interpolate ng-change, but I doubt anyone will think of doing that (as they would for ngBindHtml). I mean, you can do ng-change="{{ value }}" where value="listener()", but I doubt interpolations in event handlers are a common beginner error.
• ngTrueValue / ngFalseValue for checkboxes: They can have interpolation, but if the resulting expression isn't constant, they throw an error. I believe the "it has to be a constant expression" is a good enough safeguard.
• ngBindHtml: The only one worth fixing. Solved by fix(ngBindHtml): throw error if interpolation is used in expression #8824

Everything else throws an error that the developer can see in the console (including isolate scopes with a '=').

[IgorMinar]

what about nginclude?

[rodyhaddad]

@IgorMinar ngInclude falls into the category of "everything else that throws an error" if you use interpolation in it, so a developer can see where the mistake is in his console.

This is because ngInclude holds a copy of the attribute in its compile function that it will later be evaluated in its linking function.

So addAttrInterpolateDirective doesn't get a chance to remove the interpolation, like it used to in the case of ngBindHtml

[IgorMinar]

This must have changed since the last time I got burned by this. Thanks for
checking.
On Aug 28, 2014 10:17 PM, "Rodric Haddad" notifications@github.com wrote:

@IgorMinar https://github.com/IgorMinar ngInclude falls into the
category of "everything else that throws an error" if you use interpolation
in it, so a developer can see where the mistake is in his console.

This is because ngInclude holds a copy of the attribute in its compile
function

angular.js/src/ng/directive/ngInclude.js

Line 181 in 5f3f25a

var srcExp = attr.ngInclude || attr.src,

that it will later be evaluated in its linking function.

So addAttrInterpolateDirective doesn't get a chance to remove the
interpolation, like it used to in the case of ngBindHtml

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