search

jameskleeh / angular-confirm

Confirmation modal dialog for AngularJS
Apache License 2.0
150 stars 75 forks source link

Refactored by Codermar. Corrected issues with minification and strict DI #53

Closed Codermar closed 8 years ago

Codermar commented 8 years ago

Refactored by Codermar. Corrected issues with minification and strict DI. Unit test for the original file did not actually run. The code is essentially the same. I just added explicit injection and just code styled it.

jameskleeh commented 8 years ago

There are no issues with minification. I provided a minified file if you don't have the ability to add the annotations at build time yourself.

So beyond that, can you explain what these changes are attempting to do?

simison commented 8 years ago

Looks a lot like https://github.com/johnpapa/angular-styleguide/blob/master/a1/README.md

(...which is a good thing)

Codermar commented 8 years ago

Yes, I did code styled it based on those guidelines. I added explicit injection so we don't have to rely on running ngAnnotate on it. When I was minifying my app I was running into the error "is not using explicit annotation and cannot be invoked in strict mode" https://docs.angularjs.org/error/$injector/strictdi. It's running fine now with my changes.

simison commented 8 years ago

@Schlogen there seems to be quite a difference between different projects; some prefer handing injecting dependencies manually and some use ngAnnotate (esp. when their projects grow bigger).

I guess simply having them added like they're added in this PR would be a simple handout for those people not using ngAnnotate, and it doesn't really hurt to have them there?

Also, ngAnnotate is trying to guess where to inject stuff, but without explicit /* @ngInject */ it's gonna be just a guess. Perhaps you should add those to guide it, at least?

jameskleeh commented 8 years ago

@simison It is true there are differences. That is why I included a minified version so it will work regardless of the setup. In my opinion, it hurts readability. It is also adds slightly more work while adding features/functionality. It isn't code, it's extra fluff that serves no purpose.

Also, ngAnnotate is trying to guess where to inject stuff, but without explicit /* @ngInject */ it's gonna be just a guess.

That is simply not true. I've used several annotation preprocessors and they have all worked in the vast majority of cases without the use of explicit /* @ngInject */

jameskleeh commented 8 years ago

@Codermar Thank you for contributing to this project. In order to meet your needs of using strict DI, I would advise you add an annotation preprocessor to your build, or use the minifed version of the module. I appreciate your contributions in the area of code styling, however that is a purely subjective matter and it is my opinion that the way the code was styled previously is easier for me to read and understand. If you wish to contribute in the future, feel free to open an issue to discuss the changes before you spend time making changes.

Codermar commented 8 years ago

I use an annotation preprocessor but it is my opinion that a public library should not need it, especially if you have a clear way to avoid it like explicit injection.