directives with isolated scope and a optional function '&?' should be returned as undefined if the function is missing

[davidemoro]

Overview of the issue

It is not a bug, probably just a design issue about directives with isolated scope and optional functions passed via '&?'.

If you don't pass any function to the directive, you get a function that returns undefined instead of undefined.

Errors

No errors are thrown.

Motivation for or Use Case

If you declare a scope property as optional, probably the expected behaviour is getting undefined if you try to access this property.
This is exactly what happens when you have a '@?' binding and you don't fill in the optional attribute: you get undefined trying to access $scope.property.

The way to understand if there is a function parameter is inspecting the attrs.
So this seems a special case not motivated for '&?'.

Angular Version(s)

All versions, no regression.

Browsers and Operating System

All browsers and operting systems.

Suggestion

In order to get a common behaviour for optional parameters you could just add the following control to

angular.js/src/ng/compile.js

Line 1463 in 4a6a3ba

case '&':

:
if (optional && !attrs[attrName]) {
return;
}

If you find that this proposal is ok I can make a pull request from: https://github.com/davidemoro/angular.js/blob/master/src/ng/compile.js

I've just run the tests and all seems fine.

[btford]

Thanks for this! I think this will take some serious consideration since changes to $compile tend to have far-reaching effects and unintended consequences.

I don't have a particularly strong opinion on this.

@IgorMinar @caitp @tbosch what do you think?

[caitp]

Right now, the compiler doesn't really track whether or not an expression makes a function call or not (or how many, as it could be more than one). I don't feel like this really makes a lot of sense.

If we did force an expression to do something meaningful (for some definition of "meaningful", by throwing if it doesn't), this would break some apps.

I guess it's a tradeoff between notifying people that they might be doing something they don't expect to do, but I think that given how hard it is to tell if an expression is doing something useful, it's probably not a good idea to change this.

edit: It isn't that bad to make the "optional" flag actually do something for & properties, though. That seems okay with me.

edit again: actually, making these optional would still be a breaking change for directives using optional-flagged expression properties, because it would require the directive to test for the presence of the scope property before evaluating the expression. This seems bad.

[IgorMinar]

I agree that the current behavior is weird. It is not intentional.

We should fix this in 1.3

[davidemoro]

Ok, how can I help you? I want to dive into angular and there are tickets that needs a PR, so I would be glad to contribute.
I just need a mentor that reviews my code

[caitp]

I'm happy to review, although it sounds like I have a slightly different idea of how this should be implemented.

[davidemoro]

@caitp: I agree with you that this change might breaks existing directives with optional expressions and I perfectly understand your concerns :)
In this particular case we are not changing an intentional and documented behaviour but we are just fixing a weird situation: consider that all existing books or docs suggest that the optional flag makes things optional. Do you think that the risk could be acceptable and what it your different idea of implementation?

Anyway the most important thing is make the situation consistent and fixing this issue keeping things as simple as possible since the $compile is just enough complex.

And how can I contact you if I have some questions? For example asking for existing code conventions, where put the tests, etc.

[caitp]

@davidemoro you can get in touch with me, just CC me on github, shoot me an email (which is public in my profile), or find me on #angularjs.

Re: the implementation:

Right now, we aren't really doing much if a property is optional. For instance, with = properties, if the value of the attribute is undefined (IE the attribute is not specified), the compiler doesn't really tell you you're doing anything wrong. the parentGet() function is angular.noop(), and the only time a problem happens is when you try and assign the scope variable.

Similarly, not much is happening for @ expressions. @ will register an observe and listen to Attributes.$set(), regardless of whether or not the optional flag is set.

I think we need to sort of figure out how we want to deal with the optional expressions. In my mind, it should be invisible to the directive whether or not the attribute is specified or not, since it generally seems to be so far. @IgorMinar do you have some ideas about which direction to take this in, and how strict it should be?

[davidemoro]

Ok, let me know which is the definitive direction to take

[ashclarke]

I was just about to make this "fix/feature" into a PR too :(

I guess the best way to test whether these are supplied for now is to do the attrs[attrName] check in your directive?

[davidemoro]

@ashclarke: yes, the best way is checking the attrs

[Izhaki]

Has anyone wrote a PR for this?

I agree that '&' binding should account for the optional option (&?).

For instance, consider the following:

<input typeahead typeahead-on-hot-key="onHotKey( $event )" />

vs:

<input typeahead />

Sometimes attributes as typeahead-on-hot-key are optional - clients may or may not wish to provide a delegate.

So for a scope with:

scope: {
    onHotKey: '&typeaheadOnHotKey'
}

Currently the compiler always returns a $parse getter, even if the attribute was never defined, and you can even call it and it will return undefined.

This means one has to check manually to see if the attribute exists, and override the binding of the compiler.

[caitp]

I'll write a PR for this, but I think it's fine the way it is.

[yellow1912]

I think there should be a way to check if the function is passed in or not, right now it's just too weird. I have to manually check if the attribute exists.

[sramilli]

Hi,
i still see this behaviour in Angular 1.5.8
i´m passing an optional function to a sub-sub-component with "&?" in both steps.
The first sub-component detects "undefined", but the sub-sub-component got a function instead.

In my sub-component's template i would like to avoid having some logic toggling between
<sub-sub-component some-attribute="$ctrl.anAttribute">.......</...>
and
<sub-sub-component some-attribute="$ctrl.anAttribute" optional-function="$ctrl.myFunction()">.....</...>

As a workaround i use instead "=" as shown
bindings: { myFunction: "=" }

[ashclarke]

@sramilli I had the exact same issue today (1.5.8 too). Workaround I applied was the same too.

(Fun fact: I came to this thread again via Google, and then got freaked out just now because I got this notifcation. Wasn't until I visited GitHub to reply that I realised that I'd already participated in this issue over 2 years ago.)

[tabanliviu]

@davidemoro @caitp I'd like to mention that we recently hit upon this issue during our upgrade from 1.3 to 1.5 and our unit tests did not capture this behavior because in 1.3 there was no difference in the outcome between using a directive without the optional attribute vs having an optional value that is undefined.

I think this should have been marked as a breaking change or at least had some note about potential breaking change.

I admit I didn't read the full change log line by line and only looked at the breaking changes section but still looking back I would have preferred some hint about this change.

[tommck]

This problem still exists in 1.6.10

Optional expression is always a function