fix($compile): do not initialize optional '&' binding if attribute not specified #9216

caitp · 2014-09-22T17:58:12Z

Also, do not set up an expression in scope if the '&' binding is optional.

BREAKING CHANGE:

Previously, '&' expressions would always set up a function in the isolate
scope. Now, if the binding is marked as optional and the attribute is not
specified, no function will be added to the isolate scope.

Finally, if the '&' expression is not optional, and the attribute is not
specified, then rather than the function being essentially a NOOP, it will
instead throw an error indicating to the programmer that a required attribute
was not specified.

Closes #6404

caitp · 2014-09-22T17:59:36Z

So, the error content isn't very good, I'm not sure what else to add to it really.

I feel like this will surprise people, which isn't very nice :( Also it's another breaking change, so we probably can't really get this into 1.3.

However it has been asked for, and it probably is the expected behaviour.

/cc @IgorMinar can you take a look and decide if we want to do this or not?

gkalpak · 2014-09-22T18:26:11Z

docs/content/error/$compile/reqattr.ngdoc

+@description
+
+If a directive requires an attribute to be present, by specifying a non-optional isolate scope binding for that attribute, and the attribute
+not present, this error is thrown.


is not present

lgalfaso · 2014-09-22T19:19:09Z

I think this can cause more than one WTF moment... that said, what the patch does is closer to what it is expected... Can this be made an opt-in?

caitp · 2014-09-22T19:20:58Z

we probably have to make it opt-in, otherwise it's a breaking change :( will see if we even want to do this though

caitp · 2014-09-22T19:23:59Z

The problem with opt-in is, nobody will ever opt into it. The problem with not opt-in is, it breaks apps. It's an endless struggle .v.

IgorMinar · 2014-10-02T19:34:55Z

it's too late for this change to get into 1.3.

making this an opt-in is not a great option because as soon as you come across a third-party component that doesn't specify some required attributes in its template you have to opt-out.

I do agree that it's a good change though.

e-oz · 2014-12-15T23:28:03Z

It will break a lot of existing directives... What is profit of that change? If attribute is not marked as optional but in code of directive it is optional (sometimes even with checks, if it's function in scope.variable) - it will work.
I see only profit of this change is more clean and less buggy code in future (maybe not only this - let me know please), but I think "breaking change" in stable branch is too high price for it.

caitp · 2014-12-15T23:44:20Z

we all think so --- but it is what it is

googlebot · 2014-12-15T23:44:22Z

CLAs look good, thanks!

petebacondarwin · 2015-01-25T02:08:58Z

@caitp - I am also feeling uncomfortable about this change.
How about this compromise:

if the expression is not optional then we leave the behaviour as-is (i.e. we just pass a noop to the scope)
if the expression is optional (new feature) then we do not define it on the scope

This way, we do not break existing directives; plus we provide a way for directive developers to specify optional expressions and easily check for their existence in the template, etc.

What do you think?

(cc @IgorMinar)

caitp · 2015-01-30T16:08:50Z

updated, PTAL

caitp · 2015-01-30T16:10:21Z

test/ng/compileSpec.js

+    it('should not initialize scope value if optional expression binding with Object.prototype name is not passed', inject(function($compile) {
+      compile('<div my-component></div>');
+      var isolateScope = element.isolateScope();
+      expect(isolateScope.constructor).toBe($rootScope.constructor);


The Object.prototype stuff is optional --- I mean, it's basically going to do the wrong thing no matter what, so it's kind of weird. One option is to just always assign to undefined instead of just breaking, I guess?

petebacondarwin · 2015-01-30T20:52:19Z

Great - taking a look!

…t specified BREAKING CHANGE: Previously, '&' expressions would always set up a function in the isolate scope. Now, if the binding is marked as optional and the attribute is not specified, no function will be added to the isolate scope. Closes angular#6404

jbedard · 2015-01-30T21:10:38Z

src/ng/compile.js

            parentGet = $parse(attrs[attrName]);
+
+            // Don't assign noop to destination if expression is not valid
+            if (parentGet.noop && optional) break;


This will only happen if a non-string/function is passed to $parse. When would that happen?

petebacondarwin · 2015-01-30T21:45:12Z

Not so easy to cherry pick to 1.3.x ...

caitp · 2015-01-30T21:46:38Z

i'm not sure we should backport it

petebacondarwin · 2015-01-30T21:56:39Z

I realise after trying to do so manually that it is effectively a breaking change. So yes, you are right

cburgdorf · 2015-11-24T13:05:01Z

Sorry for digging out an old thread. I noticed that there is a breaking change regarding =? between 1.3 and 1.4 as well and I would like to figure out if that's due to what is described in this issue. It seems this issue is only referring to & bindings but it seems to be related.

This example throws with an exception when you try to toggle the second zippy because it doesn't use the open binding and the binding isn't marked as optional. The example is based on 1.3.

http://plnkr.co/edit/Ef2iKOde3TmGzJHRGEhE?p=preview

The same example using 1.4 does not throw however.

http://plnkr.co/edit/cWxO6shBz7OaiewCDMHp?p=preview

/cc @PascalPrecht

petebacondarwin · 2015-11-24T13:31:12Z

That is weird @cburgdorf - can you open a new issue for it. I think it is a bug.

cburgdorf · 2015-11-24T13:38:29Z

Thanks for the fast reply! I opened this one #13367

caitp added component: $compile type: bug severity: confusing labels Sep 22, 2014

caitp added this to the 1.3.0 milestone Sep 22, 2014

caitp force-pushed the issue-6404 branch from e43361b to 28907bf Compare September 22, 2014 18:00

mary-poppins added cla: yes and removed cla: no labels Sep 22, 2014

caitp force-pushed the issue-6404 branch from 28907bf to 6922ffb Compare September 22, 2014 18:24

gkalpak reviewed Sep 22, 2014
View reviewed changes

caitp force-pushed the issue-6404 branch from 6922ffb to 1df4e70 Compare September 22, 2014 18:29

Narretz added the needs: breaking change label Sep 25, 2014

jeffbcross force-pushed the master branch from dbbabb9 to 27d1234 Compare September 27, 2014 00:06

IgorMinar modified the milestones: Backlog, 1.3.0 Oct 2, 2014

IgorMinar added the frequency: moderate label Oct 2, 2014

jeffbcross force-pushed the master branch from cad9560 to f294244 Compare October 2, 2014 22:09

btford force-pushed the master branch from 9011a65 to 46db47b Compare October 6, 2014 22:54

jeffbcross force-pushed the master branch 4 times, most recently from e8dc429 to e83fab9 Compare October 10, 2014 17:38

petebacondarwin force-pushed the master branch from 4dd5a20 to 998c61c Compare October 19, 2014 10:58

lgalfaso added the 1.4-candidate label Nov 28, 2014

petebacondarwin modified the milestones: Backlog, 1.4.x Dec 15, 2014

petebacondarwin modified the milestones: 1.4.x, Backlog Dec 15, 2014

petebacondarwin assigned caitp Jan 30, 2015

caitp force-pushed the issue-6404 branch from d050783 to bea0482 Compare January 30, 2015 16:07

caitp changed the title ~~fix($compile): throw when non-optional '&' binding is used but not specified~~ fix($compile): do not initialize optional '&' binding if attribute not specified Jan 30, 2015

caitp reviewed Jan 30, 2015
View reviewed changes

jbedard reviewed Jan 30, 2015
View reviewed changes

caitp force-pushed the issue-6404 branch from bea0482 to 4f55ebf Compare January 30, 2015 21:15

caitp closed this in 6a38dbf Jan 30, 2015

cburgdorf mentioned this pull request Nov 24, 2015

Possible bug: Optional binding =? not required anymore in 1.4 where it is in 1.3 #13367

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($compile): do not initialize optional '&' binding if attribute not specified #9216

fix($compile): do not initialize optional '&' binding if attribute not specified #9216

caitp commented Sep 22, 2014

caitp commented Sep 22, 2014

gkalpak Sep 22, 2014

lgalfaso commented Sep 22, 2014

caitp commented Sep 22, 2014

caitp commented Sep 22, 2014

IgorMinar commented Oct 2, 2014

e-oz commented Dec 15, 2014

caitp commented Dec 15, 2014

googlebot commented Dec 15, 2014

petebacondarwin commented Jan 25, 2015

caitp commented Jan 30, 2015

caitp Jan 30, 2015

petebacondarwin commented Jan 30, 2015

jbedard Jan 30, 2015

petebacondarwin commented Jan 30, 2015

caitp commented Jan 30, 2015

petebacondarwin commented Jan 30, 2015

cburgdorf commented Nov 24, 2015

petebacondarwin commented Nov 24, 2015

cburgdorf commented Nov 24, 2015

fix($compile): do not initialize optional '&' binding if attribute not specified #9216

fix($compile): do not initialize optional '&' binding if attribute not specified #9216

Conversation

caitp commented Sep 22, 2014

caitp commented Sep 22, 2014

gkalpak Sep 22, 2014

Choose a reason for hiding this comment

lgalfaso commented Sep 22, 2014

caitp commented Sep 22, 2014

caitp commented Sep 22, 2014

IgorMinar commented Oct 2, 2014

e-oz commented Dec 15, 2014

caitp commented Dec 15, 2014

googlebot commented Dec 15, 2014

petebacondarwin commented Jan 25, 2015

caitp commented Jan 30, 2015

caitp Jan 30, 2015

Choose a reason for hiding this comment

petebacondarwin commented Jan 30, 2015

jbedard Jan 30, 2015

Choose a reason for hiding this comment

petebacondarwin commented Jan 30, 2015

caitp commented Jan 30, 2015

petebacondarwin commented Jan 30, 2015

cburgdorf commented Nov 24, 2015

petebacondarwin commented Nov 24, 2015

cburgdorf commented Nov 24, 2015