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 merging of two ngIfs in replace mode. #10733

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@kubawalinski

kubawalinski commented Jan 13, 2015

I noticed an error while working with ngIf in my project. It is reproduced here: http://codepen.io/kubawalinski/pen/EaWjxe?editors=101. In short, when 2 ngIfs need to be merged the merge is done incorrectly (it just concatenates the conditions with a space character). This is an attempt to fix this by concatenating with the ' && ' string, so that both conditions are checked and both need to be true for the ngIf to kick in.

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Jan 13, 2015

Member

I do not know if I want to go this route... if we start adding special rules for every directive, then sooner or later we will end up with too many rules on how attributes should be merged

Member

lgalfaso commented Jan 13, 2015

I do not know if I want to go this route... if we start adding special rules for every directive, then sooner or later we will end up with too many rules on how attributes should be merged

@kubawalinski

This comment has been minimized.

Show comment
Hide comment
@kubawalinski

kubawalinski Jan 13, 2015

Do you have any other idea for fixing this issue?

kubawalinski commented Jan 13, 2015

Do you have any other idea for fixing this issue?

@kubawalinski

This comment has been minimized.

Show comment
Hide comment
@kubawalinski

kubawalinski Jan 13, 2015

The way I see it ngIf is a special directive when it comes to merging its conditions and as such it should be treated differently in the code. How else can we get this working correctly?

kubawalinski commented Jan 13, 2015

The way I see it ngIf is a special directive when it comes to merging its conditions and as such it should be treated differently in the code. How else can we get this working correctly?

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Jan 13, 2015

Contributor

Wouldn't the same be true for ngHide and ngShow?

Contributor

realityking commented Jan 13, 2015

Wouldn't the same be true for ngHide and ngShow?

@kubawalinski

This comment has been minimized.

Show comment
Hide comment
@kubawalinski

kubawalinski Jan 13, 2015

@realityking You're absolutely right, ngShow, ngHide and also ngDisabled, ngOpen. Probably some others too, but this is what I gather from a quick look at the list of built-in directives. Basically it would affect any directive with a boolean condition. Anybody recalls other such directives apart from the ones already mentioned above?

I would be happy to add all identified directives to the fix, but first I would like the community to help me find the best solution to this problem.

kubawalinski commented Jan 13, 2015

@realityking You're absolutely right, ngShow, ngHide and also ngDisabled, ngOpen. Probably some others too, but this is what I gather from a quick look at the list of built-in directives. Basically it would affect any directive with a boolean condition. Anybody recalls other such directives apart from the ones already mentioned above?

I would be happy to add all identified directives to the fix, but first I would like the community to help me find the best solution to this problem.

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Jan 13, 2015

Contributor

Agree with @lgalfaso. Also it should be noted that we recommend against using the replace option.

Contributor

btford commented Jan 13, 2015

Agree with @lgalfaso. Also it should be noted that we recommend against using the replace option.

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Jan 13, 2015

Member

Angular has its limitations whenever there are multiple directives in the
same element, and the use of replace: true makes these limitations easy to
spot. In this specific case, we cannot merge the ngif statements as that is
not the right behavior. If you only keep the outer ngif, then every time
the expression turns to true, then a new controller for the custom
directive is created. If you only keep the inner ngif, then even when the
expression changes, the same controller is kept. The reason being very
simple, the outer ngif controls the creations of the custom directive and
the inner ngif only exists when the custom directive is present. This makes
it easy to understand that merging is not the right solution.

There are other directives that also have issues getting merged, an
example that very hard to know what to do is ng-class.

These are some of the reasons why replace: true is deprecated (and its use
discouraged).

I am sorry that you are running into this issue, but there is currently no
way to make it work both ways (this is one expression controlling the
creation of the custom directive controller and not the other). To make
this compile, you will have to think what behavior you want, and expose the
outer expression inside the directive or the inner expression outside of it.

Member

lgalfaso commented Jan 13, 2015

Angular has its limitations whenever there are multiple directives in the
same element, and the use of replace: true makes these limitations easy to
spot. In this specific case, we cannot merge the ngif statements as that is
not the right behavior. If you only keep the outer ngif, then every time
the expression turns to true, then a new controller for the custom
directive is created. If you only keep the inner ngif, then even when the
expression changes, the same controller is kept. The reason being very
simple, the outer ngif controls the creations of the custom directive and
the inner ngif only exists when the custom directive is present. This makes
it easy to understand that merging is not the right solution.

There are other directives that also have issues getting merged, an
example that very hard to know what to do is ng-class.

These are some of the reasons why replace: true is deprecated (and its use
discouraged).

I am sorry that you are running into this issue, but there is currently no
way to make it work both ways (this is one expression controlling the
creation of the custom directive controller and not the other). To make
this compile, you will have to think what behavior you want, and expose the
outer expression inside the directive or the inner expression outside of it.

@kubawalinski

This comment has been minimized.

Show comment
Hide comment
@kubawalinski

kubawalinski Jan 14, 2015

@lgalfaso Thanks for this in-depth explanation. I did not know that replace was deprecated. This decision makes a lot of sense now that you have laid it out so clearly.

My example (http://codepen.io/kubawalinski/pen/EaWjxe?editors=101) was deliberately simplified, but in real life the situation that I encountered was that I was using somebody else's directive from npm and I wanted to use ngIf on it, but it failed due to the fact that there was an inner ngIf on a root element inside that directive.

I guess the way forward in such situations is to use ngShow if a directive uses ngIf and vice versa. It's not ideal, as I know some developers like to control their resulting markup pretty tightly. However, I see no other way around such an issue at this time. Would you guys agree?

kubawalinski commented Jan 14, 2015

@lgalfaso Thanks for this in-depth explanation. I did not know that replace was deprecated. This decision makes a lot of sense now that you have laid it out so clearly.

My example (http://codepen.io/kubawalinski/pen/EaWjxe?editors=101) was deliberately simplified, but in real life the situation that I encountered was that I was using somebody else's directive from npm and I wanted to use ngIf on it, but it failed due to the fact that there was an inner ngIf on a root element inside that directive.

I guess the way forward in such situations is to use ngShow if a directive uses ngIf and vice versa. It's not ideal, as I know some developers like to control their resulting markup pretty tightly. However, I see no other way around such an issue at this time. Would you guys agree?

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Jan 14, 2015

Member

The use of ngIf and ngShow combination is a solution. BTW (and I know you
are using a third party component), using an ngIf outside and ngShow inside
works best.

Member

lgalfaso commented Jan 14, 2015

The use of ngIf and ngShow combination is a solution. BTW (and I know you
are using a third party component), using an ngIf outside and ngShow inside
works best.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Jan 14, 2015

Member

BTW, (if your usecase allows it), you could wrap the directive in a parent element and put ngIf on that.

Member

gkalpak commented Jan 14, 2015

BTW, (if your usecase allows it), you could wrap the directive in a parent element and put ngIf on that.

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Jan 14, 2015

Member

BTW, (if your usecase allows it), you could wrap the directive in a parent element and put ngIf on that.

Please do not do this. If this in fact works, then it is an oversight and may break in the future.

I am closing this issue now

Member

lgalfaso commented Jan 14, 2015

BTW, (if your usecase allows it), you could wrap the directive in a parent element and put ngIf on that.

Please do not do this. If this in fact works, then it is an oversight and may break in the future.

I am closing this issue now

@lgalfaso lgalfaso closed this Jan 14, 2015

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Jan 14, 2015

Member

Hm...it does indeed work, but why is it an oversight ? Why do you say it shouldn't work ?

Just to make sure we are on the same page, I am talking about something like this:

HTML

<div ng-if="expr1">
  <test show="expr2"></test>
<div>

JS

.directive('test', function testDirective() {
  return {
    template: '<span ng-if="show">Hello, world !</span>',
    scope: {show: '='},
    replace: true
  };
}

Demo fiddle

So, @lgalfaso, are you saying the above shouldn't be done (and might break in the future) ? Why is that ?
(There is a good chance I am missing something, so thx for any insights into this :))

Member

gkalpak commented Jan 14, 2015

Hm...it does indeed work, but why is it an oversight ? Why do you say it shouldn't work ?

Just to make sure we are on the same page, I am talking about something like this:

HTML

<div ng-if="expr1">
  <test show="expr2"></test>
<div>

JS

.directive('test', function testDirective() {
  return {
    template: '<span ng-if="show">Hello, world !</span>',
    scope: {show: '='},
    replace: true
  };
}

Demo fiddle

So, @lgalfaso, are you saying the above shouldn't be done (and might break in the future) ? Why is that ?
(There is a good chance I am missing something, so thx for any insights into this :))

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Jan 14, 2015

Member

@gkalpak I thought you meant that if in the outer element you have an attribute named ngIf (not ng-if, but ngIf) then the ngIf directive would get called anyhow and the attributes not merged

Member

lgalfaso commented Jan 14, 2015

@gkalpak I thought you meant that if in the outer element you have an attribute named ngIf (not ng-if, but ngIf) then the ngIf directive would get called anyhow and the attributes not merged

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Jan 14, 2015

Member

I always refer to directives by their normalized named 😃
Sorry, for the confusion :)

Member

gkalpak commented Jan 14, 2015

I always refer to directives by their normalized named 😃
Sorry, for the confusion :)

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