New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

@caitp
Contributor

caitp commented Sep 22, 2014

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 caitp added this to the 1.3.0 milestone Sep 22, 2014

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Sep 22, 2014

Contributor

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?

Contributor

caitp commented Sep 22, 2014

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?

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

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Sep 22, 2014

Member

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?

Member

lgalfaso commented Sep 22, 2014

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

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Sep 22, 2014

Contributor

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

Contributor

caitp commented Sep 22, 2014

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

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Sep 22, 2014

Contributor

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.

Contributor

caitp commented Sep 22, 2014

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

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Oct 2, 2014

Member

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.

Member

IgorMinar commented Oct 2, 2014

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.

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

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

@e-oz

This comment has been minimized.

Show comment
Hide comment
@e-oz

e-oz Dec 15, 2014

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.

e-oz commented Dec 15, 2014

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

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 15, 2014

Contributor

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

Contributor

caitp commented Dec 15, 2014

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

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Dec 15, 2014

CLAs look good, thanks!

googlebot commented Dec 15, 2014

CLAs look good, thanks!

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jan 25, 2015

Member

@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)

Member

petebacondarwin commented Jan 25, 2015

@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 caitp changed the title from fix($compile): throw when non-optional '&' binding is used but not specified to fix($compile): do not initialize optional '&' binding if attribute not specified Jan 30, 2015

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Jan 30, 2015

Contributor

updated, PTAL

Contributor

caitp commented Jan 30, 2015

updated, PTAL

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);

This comment has been minimized.

@caitp

caitp Jan 30, 2015

Contributor

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?

@caitp

caitp Jan 30, 2015

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jan 30, 2015

Member

Great - taking a look!

Member

petebacondarwin commented Jan 30, 2015

Great - taking a look!

fix($compile): do not initialize optional '&' binding if attribute no…
…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 #6404
Show outdated Hide outdated src/ng/compile.js

@caitp caitp closed this in 6a38dbf Jan 30, 2015

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jan 30, 2015

Member

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

Member

petebacondarwin commented Jan 30, 2015

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

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Jan 30, 2015

Contributor

i'm not sure we should backport it

Contributor

caitp commented Jan 30, 2015

i'm not sure we should backport it

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jan 30, 2015

Member

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

Member

petebacondarwin commented Jan 30, 2015

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

@cburgdorf

This comment has been minimized.

Show comment
Hide comment
@cburgdorf

cburgdorf Nov 24, 2015

Contributor

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

Contributor

cburgdorf commented Nov 24, 2015

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

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Nov 24, 2015

Member

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

Member

petebacondarwin commented Nov 24, 2015

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

@cburgdorf

This comment has been minimized.

Show comment
Hide comment
@cburgdorf

cburgdorf Nov 24, 2015

Contributor

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

Contributor

cburgdorf commented Nov 24, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment