feat($compile/ngBind): don't add binding info if not enabled

[petebacondarwin]

The compiler and ngBind directives add binding information (ng-binding
CSS class and $binding data object) to elements when they are bound to
the scope. This is only to aid testing and debugging for tools such as
Protractor and Batarang. In production this is unneccesary and add a
performance penalty.

This change disables adding this binding information by default. You can
enable it by calling $compilerProvider.enableBindingInfo() in a module
config() block.

BREAKING CHANGE:

The ng-binding CSS class and the $binding data object are no longer
attached to DOM elements that have a binding to the scope, by default.
If you relied on this feature then you can re-enable it in one of your
application modules:

someModule.config(['$compileProvider', function($compileProvider) {
  $compileProvider.enableBindingInfo();
}]);

[petebacondarwin]

Closes #8423

[petebacondarwin]

Note that previous commits moved the adding of the CSS class into the compile function from the post-link function to aid performance. But since we are now removing these calls altogether during production there is little benefit in this performance fix.

[lgalfaso]

LGTM

[petebacondarwin]

This could be moved into Protractor (and Batarang) itself?

[juliemr]

Yes, definitely. We'll just check if the function (enableBindingInfo) exists for backward compatibility.

[juliemr]

Possibly unrelated: do we know what this todo was for? Can we remove?

[IgorMinar]

I don't know. +1 for removing.

[IgorMinar]

how about removing all the $scope info from data as well? would that be a breaking change? Angular doesn't need it and we expose reference to scope via controllers and directives, so there shouldn't be a strong need for it.

this change would make debugging much harder though and in that case we might want to turn on the debugging mode by default in non-minified version.

I was also thinking about how could we easily tell if we are in minified or non-minified mode, and one solution that came to my mind was to check name of some helper fn. if (isDefined.name === 'equals') { /* non-minified mode */ }

too crazy? thoughts?

[petebacondarwin]

@IgorMinar - regarding turning this on for non-minified builds:

@tbosch made a good point that we should be running E2E tests against minified code, so we would still need to have a switch to turn it on for most Protractor scenarios. Given that, I don't think there is much point in spending a long time working out how to tell if we are minified or not.

More simply we could have a module (or just a config block), which turns on the bindingInfo. This would live in a separate source file and include it only in the non-minified build.

[IgorMinar]

@petebacondarwin if we don't publish $scope when debugging is off, then you do want it on by default in non-minified build

[tbosch]

We could to the following:

1. separate the debugging stuff into a separate module that lives in a separate file
2. change protractor and batarang as well to inject that module into every app
3. allow the module to be loaded by developers manually as well, just like they load any other module

Regarding removing all the $scope info from data as well in debug mode:

• then functions like jqLite.scope(), jqLite.inheritedScope(), ... won't work any more
• so this would be a breaking change
• I like it!

[juliemr]

My 2c - jqLite.scope() is super useful for debugging. Telling devs that they have to restart with a separate module included to do that is a definite impediment to easy debugging. If we decide to make this change it has to come with crystal clear and easy ways to turn debugging back on.

[petebacondarwin]

So, say we have a module called ngDebug? This module will turn on all the various debug features such as ngBind info, checking localDigests and exposing scope info on the DOM. It could even modify the $log service to output more info.

Turning on the debug stuff would then be simply a case of loading this and adding a dependency on it to the app module.

There are two approaches we could consider:

• Angular always attempts to load ngDebug module (just like we do at the moment with ngLocale) if it is loaded. Then turning on the debug features would be as simple as including a script tag in your app to load a file (say angular-debug.js that we would provide) in your app.
• We include ngDebug (it would be small) in all the builds of angular.js as a matter of course. Then it would be easy to turn on the debug features by simply referencing ngDebug as a dependency on the app module. This would work better with tools like Protractor and Batarang as they can simply inject a dependency on this preloaded module before the app begins.

[shahata]

+1 for not forcing devs to reload the page with some injected module in order to access jqLite angular helpers.

[IgorMinar]

can we just get rid of this fn and use jqLiteAddClass instead? that should work for all elements regardless of whether they are svg or not

[IgorMinar]

In the current state I don't see how this will be off by default in production but on in development. That bit is missing. I left a few other comments on the individual commits. nothing major.

[jbedard]

Will the jQuery/jqLite .scope and .isolateScope methods just return undefined in non-debug mode? I've never seen those methods documented as "debug information" and used them quite a bit...

[petebacondarwin]

@IgorMinar I thought we agreed not to change it based on the minification?

I think it would actually lead to more confusion because people might use scope () etc in development then get upset that it crashes in production.

They should proactively turn it on using $compileProvider if they want to use it in their app or use reloadWithDebugInfo when debugging.

[petebacondarwin]

@jbedard what is your production use case for scope() etc?

I would think that a noop would be more confusing since the app semantic would quietly change.

[jbedard]

I have a few cases such as...

$rootElement.on("frequent-propogated-event", ["possibly a child selector", ]function(e) {
   if (someCheck(e.target) or someOtherCheck(e.keyCode)) {
      var scope = $(this).scope();
      scope.$apply(function() {
          //something which uses local scope info such as ngRepeat variables
      });
   }
});

Most of those I can easily change to fetch the scope differently because the directive controls the child elements (and I can manage the DOM<=>scope myself). What about cases such as ngRepeat though? Does ngRepeat provide a way to get the scope based on the repeated DOM element? I know this pattern isn't really encouraged and using a local ngEvent per repeated element would be preferred but that is often too expensive (at link time due to more linking + jQuery.on and at event firing time due to the unnecessary digests when someCheck is normally false).

It would be very nice to have no jQuery data by default during compile/link though. The scope (and controllers?) could be put directly on the DOM instead of in jQuery data...

[petebacondarwin]

@jbedard in this case you should just use $rootScope as it has the same effect unless you are hoping to make use of localApply in which case you really should be in a closure containing the appropriate scope anyway

[jbedard]

@petebacondarwin my point was that within the .apply local scope information is used such as the current ngRepeat variables. Which scope calls .apply doesn't matter...

[petebacondarwin]

@jbedard - Oh I didn't notice the content of that comment.
I think there was some work on a directive that handled delegated events...

[vojtajina]

I rebased this PR on the latest master and addressed all the comments.
Subtle changes (such as removing spaces, single quotes etc) were squashed into Pete's original commits.
Bigger changes that might possibly introduce problems were added as separate commits.

[vojtajina]

It's here https://github.com/vojtajina/angular.js/tree/pr-8430

[jbedard]

@petebacondarwin but how would that directive fetch the scope of the delegated event target without .scope()?

[vojtajina]

Talked with @IgorMinar and we decided to enable the debugging by default. That means this is not a breaking change.

We also talked about the possibility to make the debug info enabled in non-minified build and disabled in the minified build. That could however lead to apps working in development and crashing in production.

[vojtajina]

Thus I'm also renaming the config method to $compileProvider.disableDebugInfo().

[vojtajina]

One more change, in order to follow convention of other APIs (eg. chaining), I'm gonna call this method $compileProvider.debugInfoEnabled(bool).

[IgorMinar]

But we also agreed to print into console what mode we are in when we
bootstrap and post a link to our docs with info about the implications,
configuration, etc.
On Aug 26, 2014 6:13 PM, "Vojta Jina" [email protected] wrote:

One more change, in order to follow convention of other APIs (eg.
chaining), I'm gonna call this method
$compileProvider.debugInfoEnabled(bool).

—
Reply to this email directly or view it on GitHub
#8430 (comment).

[petebacondarwin]

Also I think @jbedard's comment about access to scope in delegated events should be addressed. Not sure how to provide this if we turn off scopes. Any ideas?

[IgorMinar]

I'm not sure how to address that one without attaching the scope or its id
directly to the DOM. I think for now you will have to keep the debugging
info on if you do event delegation.
On Aug 26, 2014 11:19 PM, "Pete Bacon Darwin" [email protected]
wrote:

Also I think @jbedard https://github.com/jbedard's comment about access
to scope in delegated events should be addressed. Not sure how to provide
this if we turn off scopes. Any ideas?

—
Reply to this email directly or view it on GitHub
#8430 (comment).

[petebacondarwin]

At least with the decision to make this optimization (removing scope info from DOM) opt-in it is less likely to break applications but we should ensure that this use case is highlighted in the docs.

[vojtajina]

So to summarize. The config API is $compileProvide.debugInfoEnabled(bool). It is enabled by default. I also reverted back to adding classes in compile when possible as it has a significant performance improvement (well, in the largetable-bp benchmark).

As long as Travis is happy, I'm gonna merge this in. If anybody wants to do a review, it's here:
#8802

[vojtajina]

Merged as c6bde52

[IgorMinar]

Yay!
On Aug 27, 2014 9:22 PM, "Vojta Jina" [email protected] wrote:

Merged as c6bde52
c6bde52

—
Reply to this email directly or view it on GitHub
#8430 (comment).

[petebacondarwin]

Thanks @vojtajina !!