Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

perf(ngSwitch): Define this directive as terminal #12079

Closed
wants to merge 1 commit into from

Conversation

dcherman
Copy link
Contributor

Since a switch statement ( and therefore ngSwitch ) can really be
thought of as simply a combination of if statements, there's no reason
that this directive should differ from ngIf in how it compiles the
element. This should also prevents needless compilation of directives
when the ngSwitch condition isn't yet matched.

Since a switch statement ( and therefore `ngSwitch` ) can really be
thought of as simply a combination of if statements, there's no reason
that this directive should differ from `ngIf` in how it compiles the
element.  This should also prevents needless compilation of directives
when the ngSwitch condition isn't yet matched.
@dcherman
Copy link
Contributor Author

Is there a way to re-trigger the build? The test that failed has absolutely nothing to do with my changes ( it's an ngAnimate test that doesn't even use ngSwitch ), so this must be an existing problem or non-deterministic test.

@lgalfaso
Copy link
Contributor

If you have the need to make ngSwitch terminal, then just add these directives

// Warning untested code
myApp.directive('ngSwitchWhen', function() { return {terminal: true}; })
.directive('ngSwitchDefault', function() { return {terminal: true}; });

and within your app, these directives will be terminal. Now, this is too big of a breaking change, so this is not going to fly.

Closing

@lgalfaso lgalfaso closed this Jun 10, 2015
@dcherman
Copy link
Contributor Author

@lgalfaso How is this situation different than ngIf?

@lgalfaso
Copy link
Contributor

@dcherman the content of an ngIf is compiled just like the content of ngSwitch is compiled. This is done regardless of the terminal property as in both cases they have transclude: 'element'. This is, the original premise that is in the description (that with the change the content of the ngSwitch would not get compiled) is false. You can check this out at http://plnkr.co/edit/SIptkEHigAkrCmusvx1u?p=preview

@dcherman
Copy link
Contributor Author

@lgalfaso So that's 100% correct, and is addressed in my other pull request ( #12078 ). I guess what I don't understand here is why this directive should not be terminal. It seems logical that it would match the behavior of an ngIf where lower priority directives would be compiled once the transclusion happens ( when the case is matched ).

As with the other PR, would you be open to consideration for this in a later version of Angular if you feel it'd be breaking?

@lgalfaso
Copy link
Contributor

If #12078 lands, and we are talking for 1.5, then this makes a lot of sense.

@dcherman
Copy link
Contributor Author

@lgalfaso Since it's looking like there's a pretty good chance that #12078 will be landing in 1.5, can we re-open this for consideration at that same time? I'm happy to add additional unit tests to verify the terminal behavior if needed

@lgalfaso
Copy link
Contributor

@dcherman if #12078 lands. Would it be possible for you to write a test that shows a behavior change with and without terminal: true ?

@dcherman
Copy link
Contributor Author

@lgalfaso So I was just revisiting this, and there is actually no change needed at all. transclude: "element" basically implies terminal, so ngSwitch already has the terminal behavior.

I guess technically that means that ngIf doesn't even need terminal, but it doesn't affect a thing. TIL.

@lgalfaso
Copy link
Contributor

@dcherman thanks for confirming this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants