ngIf with replace:true on directive template

[bertrandgressier]

Hello team,

We have a regression with the last angular version (since v1.3.0-beta.11)

In this commit https://github.com/angular/angular.js/commit/d71df9f83cd3882295ca01b1bb8ad7fb024165b6#diff-1c8fbee1402a2ef3c203def41d1960cdR97, you modified the child scope ngIf uses. Now with this modification, using ngIf on the root element of a directive template that is configured with replace:true, the $scope is not the expected one. (it's not the same)

We tried to build a reduced test case to show the regression :

• 1.3.0-beta 10 : http://jsbin.com/sakuto/9/edit
• 1.3.0-beta 11 : http://jsbin.com/yubetu/1/edit

As you can see, the first directive failed in beta 11.

What do you think ? Is it a bug or the new expected behaviour ? If so, maybe we should detail the documentation.

thx!

[clakech]

Hi, @caitp could you help us on this one? ;-)

[caitp]

since you asked nicely ^^

[caitp]

so, you can work around this by not using an element transclusion directive as the root element of the template --- I'm not sure exactly why this worked in beta 10 though, doesn't make a lot of sense.

Lets dig through the changelog and see if it's listed in any of the breaking changes

[caitp]

@petebacondarwin there are no breaking changes for $compile in beta 11, i think we forgot to add them

[caitp]

I wonder if we could make this more palatable --- like, if you have element transclusion at the root of a replacement template, it should add the original parent scope in a custom data field, and use that when getting the parent scope.

[clakech]

Thank you for your help about this! I knew your advice would be useful! ^^

We would definitely love to see a breaking change in the changelog and a new palatable behavior of angular in this use case.

We hesitate between removing replace:true and avoid using element transclusion directive as root element.

[clakech]

Ok so I added another exemple in the jsbin with a use case where the ng-if is not on the root element of the directive:

http://jsbin.com/fahoz/1/edit?html,js,output

[pocesar]

seeing this as well, since upgrading to 1.3.1. my template is

template =  '<div class="tab-content" ng-if="visible">' +
                    '<div ng-if="tab" class="tab-inner-content clearfix" ng-include="tab"></div>' +
                   '</div>';

and it broke

[clakech]

OK so someone need to propose a breaking change for this but where should we document this ? in ngIf doc ? in directive doc ? in changelog ?

[petebacondarwin]

Only just seen this one. Let me take a look this week.

[marlun78]

I had this issue too. It seems to work if you wrap the root element (with the ngIf) in a new element (without ngIf) - as a temporary fix. http://plnkr.co/edit/9wgADGTILYINqHEdY1KE?p=preview

[petebacondarwin]

Moving to 1.3.4 as I suspect that this is going to be a tricky fix (and not something that can be knocked up and suitably reviewed by Monday).

[petebacondarwin]

I have spent some time looking at this. I feel that it is not a bug in ngIf - ngIf is doing the right thing in asking for its new scope rather than guessing that it might be the parent scope.

Instead it is a more general bug regarding replace directives that have an element transclude directive at the root of their template.

This is a very hard bug to fix, since the original directive is being replaced by the element transclude directive; but that directive itself has been removed and replaced by a holder comment. So effectively we lose one layer of scope:

Normally without element transclude at the root you get:

{$scope-1}
<replace-isolated-transclude-directive>
  {$scope-2}
  <div>
    <!-- ng-if -->
      {$scope-2a}
      <ng-transclude>
        {$scope-1a}
      </ng-transclude>
  </div>
<replace-isolated-transclude-directive>

Here you can see that $scope-2 is the isolated scope of the directive, $scope-2a is the scope transcluded from $scope-2 of and $scope-1a is the transcluded scope from outside the transclusion directive.

If the ngIf is at the root of the template then $scope-2 never gets to the ngIf since by the time we are passing it through the link function, the ngIf has been compiled to a linkFn already bound to the outer $scope-1a:

{$scope-1}
<replace-isolated-transclude-directive>
  <!-- ng-if -->
    {$scope-1a}
    <ng-transclude>
      {$scope-??}
    </ng-transclude>
</replace-isolated-transclude-directive>

[petebacondarwin]

This is one of the many reasons that we have deprecated replace as it creates so many of these hard to fix corner cases.

[petebacondarwin]

@caitp's comment: https://github.com/angular/angular.js/issues/9837#issuecomment-61076800 is on the right track but the trick is how to do this in a non-hacky way that doesn't introduce more bugs elsewhere.

[matias-sandell]

Hi, has there been any progress on this, @petebacondarwin ?

[petebacondarwin]

Directives that do replace cause so many problems. Given that we can almost always work around it with non-replace directives, I am going to move this to the Ice Box for now.

[thetrevdev]

Is there any way to detect this and throw a warning?

I understand all the reasons to not fix and to deprecate replace... But as far as migrating an existing project this is a breaking change that isn't documented. https://code.angularjs.org/1.3.18/docs/guide/migration#angular-html-compiler-compile- This refers to other more broken use cases I feel.

We are migrating large apps that happened to have used replace: true and its difficult to know where we might have used transclusion. If we could at least identify or warn we could go and fix the replace attribute. Right now trying to replace every single directive across many apps would be quite difficult.

[petebacondarwin]

@thetrevdev - as a start you could do a search of your code base for directives that employ replace: true. Then look at their templates. If there is one of ng-if, ng-repeat or ng-switch on the root node of the template then you will need to fix it.

[thetrevdev]

Thats pretty difficult to do with a very large app.
I have currently made a local fork of angular that errors when it detects this case while migrating. When templates are involved and have isolate scope it marks the directives. I check here for the offending transclusion.

https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L2104

[petebacondarwin]

@thetrevdev - glad you have got something working for you. If it turns out to be very helpful perhaps you could submit a patch?

[petebacondarwin]

I think that this may be fixed by #12975

[JaKXz]

I think I'm still seeing this issue on v1.4.8. My directive had replace: true and an ng-if on the root element. This let the directive be rendered but none of the children elements had the correct scope. Removing replace: true fixed the issue.

[adamreisnz]

Also still experiencing this issue. I will look at avoiding replace: true, but it's going to be a bit of a pain.

[petebacondarwin]

@adambuczynski - other than CSS that is too constraining, are there any other reason why this is a pain?

[adamreisnz]

@petebacondarwin Yeah, CSS was one of the initial pains I ran into, but it was late at night and today's a new day, so it might turn out not as bad as I originally thought! :)

There are a few other reasons I identified yesterday thought, one is that we need to apply an ng-if condition on the parent element of the directive. Currently, we are using ng-show for that, but the drawback of that is, that the scope/controller is not re-initialized when the directive is shown. So for that reason we wanted to move to using ng-if (which led me to this issue in the first place).

In order to not have to put the ng-if logic in our main templates, we embed it in the directive itself, which judging from the comments in this thread, seems to be a common use case. However, if the directive is not replacing the parent element, it will not work as expected, as it will only remove the child element, and not the parent element. Moving the ng-if's to the main template is not an option.

Second, to avoid repetition we also apply some classes to the parent element via the directive (via attributes on the directive's template). These get merged nicely now with any classes that are applied to the directive element in the main template. With replace: false this will no longer work, and we'll be forced to apply the classes programmatically via the link function, which isn't the end of the world, but just another (somewhat redundant) step that we have to take.

Anyway, I only found out that replace: true is deprecated by stumbling upon this issue. Most, if not all of our directives use replace: true, so I will run by all of them today and see if any other issues arise if we set it to false. It would have been good to receive deprecated notices in the console though, to alert developers of the fact they are using a deprecated API.

Will report back after I've had another good look at it today, and see if I can work around them while not using replace: true. If any other pain points come up I will let you know.

[petebacondarwin]

@adambuczynski - be aware that though replace:true is deprecated, we will not remove it from Angular 1.x - it is already gone from Angular 2. What it does mean is that we are not going to spend much time trying to fix the many complex and difficult corner case problems it has.

[adamreisnz]

Thanks, yes I got that form reading more about it. I do intend to migrate to Angular 2 sooner rather than later, so figuring out now how to best use non-replacing directives would be the right step to take.

To be honest though, I don't think wanting to apply ng-if on the parent element via the directive template qualifies as a corner case. It seems like a fairly common use case.

[petebacondarwin]

It may not be "corner" but for sure it is "difficult". I haven't seen any PRs offering to fix it :-)

[adamreisnz]

@petebacondarwin Well, most directives I was able to convert painlessly, but there were the couple where some CSS tweaks were necessary. As for the one with the ng-if on it, I've managed to refactor it in a way that the ng-if can still be applied on the inner element, and with some CSS tweaks it seems to be looking and working the same now. So didn't need replace: true after all.

Do Angular 2 components offer a way to apply directives and other attributes on to the parent element via the directive templates? Because that currently seems to be the biggest drawback in Angular 1 directives, when not using replace: true.

[LarryTurtis]

It took me awhile to figure out I had this issue. I spent a fair amount of time assuming I had some sort of error in my scope configuration. It was only on a whim that I removed ng-if from the root element and discovered this to be the problem. It would be good to have some sort of warning.

[douglasg14b]

Just spent several hours trying to figure out what was wrong with my code to finally narrow it down to the ng-if. Happy there is a known issue...