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

Provide a way to turn off debugging info in production #8423

Closed
IgorMinar opened this issue Jul 30, 2014 · 19 comments
Closed

Provide a way to turn off debugging info in production #8423

IgorMinar opened this issue Jul 30, 2014 · 19 comments

Comments

@IgorMinar
Copy link
Contributor

angular sets ng-binding class and other classes as well as adds $binding object to jqLite/jQuery data storage. This info is not needed in production and is only used by tools like batarang and protractor but are causing performance issues for apps with lots of bindings.

These tools can already intercept the angular bootstrap and change configuration of the app if needed, so they could turn on the debugging info on demand.

I'm thinking that to control the behavior we could use $compileProvider and expose a setter like debugMode() that tools could use.

The debugging info should be off by default in production.

Open questions:

  • should we disable this in non-minified angular by default? are the css classes useful for development?

cc: @btford, @rev087, @juliemr

@IgorMinar IgorMinar added this to the 1.3.0 milestone Jul 30, 2014
@petebacondarwin
Copy link
Contributor

So my initial thoughts are to move all attaching of ng-binding css classes and $binding data into a function on $compile, say $compile.addBindingClass(element). Then we can swap this for a noop if in production.

@petebacondarwin
Copy link
Contributor

Also I think we should just disable this debugging information for all builds by default. It is trivial to enable if it is needed.

@petebacondarwin
Copy link
Contributor

Obviously we would need to implement updates to Protractor and Batarang to support this

@petebacondarwin
Copy link
Contributor

And also the ngScenario runner would need updating too

@caitp
Copy link
Contributor

caitp commented Jul 31, 2014

the scenario runner is deprecated anyways, so why update it?

@petebacondarwin
Copy link
Contributor

Because although it is deprecated it has not yet been removed and so would suddenly break when upgrading to the new version of AngularJS.

@rev087
Copy link

rev087 commented Jul 31, 2014

ng-inspector doesn't make use of the ng-binding classes, so this change wouldn't affect it.

Tools could also simply look for the $binding object and friends - clunky, sure, but the clunkiness is contained in tooling and not spilling over to the production lib. So I'd say it's ok to remove them.

Thanks for the heads up by the way!

@mgol
Copy link
Member

mgol commented Aug 1, 2014

Because although it is deprecated it has not yet been removed and so would suddenly break when upgrading to the new version of AngularJS.

Will the scenario runner be kept alive for Angular 1.3? Because if not, this might be a good moment to remove it...

@petebacondarwin
Copy link
Contributor

I don't believe it is removed in 1.3 - @angular/angular-core?

@juliemr
Copy link
Member

juliemr commented Aug 1, 2014

Support in Protractor should be very easy if it's just a change in $compileProvider.

I suspect that there are many little custom applications and debugging aids out there that use the ng-binding class to help them out. (A simple example, just using CSS to make bindings visible), so this should certainly be documented as a breaking change.

Is there currently any functional difference between minified and non-minified angular?

@tbosch
Copy link
Contributor

tbosch commented Aug 1, 2014

Hi,
it would be nice to have a feature that is a little more general.
E.g. for #8055 it would also be nice to add checks during development that nothing outside of the isolated component changed (via a global digest after the partial digest), but remove them in production.

Enabling those checks during protractor tests would be a good way so that an Angular user would not need to care for the configuration.

@petebacondarwin
Copy link
Contributor

@juliemr did you see the pull request #8430 and in particular where I fix up the e2e tests? Should this code move to Protractor?

@tbosch did you mean that #8430 should be more general or is that change ok and you want to provide other switches too for protractor etc?

@tbosch
Copy link
Contributor

tbosch commented Aug 4, 2014

For #8430 I would like to provide other switches for protractor as well.

However, it feels like those switches are better placed into an e2e mock module that angular provides (just like the angular-mocks module but for e2e tests) and that Angular manages. Via that, if there are bugfixes, ... we don't need to change protractor. Also, protractor should be usable for AngularDart and Angular V2 as well, without a special case for each of those.

The only problem with an e2e mock module in AngularJS is how that gets loaded. For your case of turning off the debugging information, this module is really required, as otherwise tests that select bindings will not work.

One solution could be to make the e2e mock module part of the non minified angular core build, and remove it during minification. What do you think?

@petebacondarwin
Copy link
Contributor

I think it is a good idea to package this up into a module.

From discussions with @tbosch we realised that ideally Protractor should have an adaptor architecture (similar to Karma?) which can be configured for each project that is using Protractor (in the proractor.conf.js).

The AngularJS adaptor will provide the bits that are currently hard-coded into Protractor for injecting modules into AngularJS applications, waiting for AngularJS requests to complete and providing AngularJS specific selectors. This adaptor would by default inject this "debugBindings" module into the application before running the tests.

The adaptor architecture would then free up Protractor to have different (non-hard-coded) features for AngularDart and AngularJS v2 applications.

@tbosch
Copy link
Contributor

tbosch commented Aug 4, 2014

And then for #8430 we could have another addition to that adaptor.

@juliemr
Copy link
Member

juliemr commented Aug 4, 2014

I took a look at #8430, sorry for the delay! Adapter architecture seems reasonable but I'm not sure it's worth it for 3 use cases (AngularJS, AngularDart, AngularJS v2). It seems like a lot of overhead to avoid a couple conditionals, and the set of cases isn't going to expand much.

@tbosch I think it would be helpful for this discussion to have context on what other switches you'd like to turn off for debugging/e2e testing.

@petebacondarwin
Copy link
Contributor

@juliemr - I think @tbosch meant #8055

@juliemr
Copy link
Member

juliemr commented Aug 4, 2014

Ah, thanks. I think using Protractor to output warnings about your application health (a la #8055) is a cool idea but probably needs more thinking through. How are the errors surfaced?

@petebacondarwin
Copy link
Contributor

Landed via #8802

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

No branches or pull requests

8 participants