klei / gulp-angular-filesort

Automatically sort AngularJS app files depending on module definitions and usage
MIT License
132 stars 46 forks source link

Not working due to the isDependecyUsedInAnyDeclaration check #26

Closed buunguyen closed 7 years ago

buunguyen commented 9 years ago

I have an app where a module is contained in multiple files in a folder. There's a main file which declares the module (i.e. angular.module('mod', [])) and other files that add stuff to it (e.g. angular.module('mod').controller('c1')). gulp-angular-filesort doesn't sort files correctly at all. However, if if I comment out the check isDependecyUsedInAnyDeclaration, it starts working.

I'm not exactly sure what this check is for and why it is needed. Is it an optimization? If it is, it breaks the case like my app where a module is contained in multiple files. Can you shed some light? Thank you.

joakimbeng commented 9 years ago

Does the sort order break your whole app? I.e. does Angular give you module loading errors? The scenario you describes is working for me with this module, so I need more information of what exactly is not working, and preferably with an example of your directory/file structure.

buunguyen commented 9 years ago

@joakimbeng thanks for responding. Yes, the sort order breaks the app and I also get the module loading error. My observation is that when the check exists, many files in the project aren't added to the toSort list, resulting in them not being sorted.

I will try to reproduce the issue in a clean project and share more information when I can.

buunguyen commented 9 years ago

You can see the issue in this repo. Just run npm install && gulp to generate index.html. There are 3 modules, users, core and app. app depends on users which depends on core. But the injected script order is: user -> core -> app instead of core -> user -> app.

buunguyen commented 9 years ago

@joakimbeng, just want to check in if there's any update. Thanks!

simon04 commented 9 years ago

I assume that there's a misunderstanding on the purpose of the sorting: this gunt module sorts modules according "define before use", but does not sort module dependencies in front of modules using those: See https://github.com/klei/gulp-angular-filesort/issues/7#issuecomment-51068064

Maybe that should be clarified in the README.

buunguyen commented 9 years ago

@simon04 I would argue that "use" = "depend" (i.e. A uses B is similar to A depends on B). So the plugin should support dependencies as well. Otherwise, it wouldn't work even for a simple (yet realistic) example in the repo I provided.

buunguyen commented 9 years ago

@simon04 I would argue that "use" = "depend" (i.e. A uses B is similar to A depends on B). So the plugin should support dependencies as well. Otherwise, it wouldn't work even for a simple (yet realistic) example in the repo I provided.

simon04 commented 9 years ago

Issue #25 is related, see especially https://github.com/klei/gulp-angular-filesort/issues/25#issuecomment-87777133.

"Define" has to come before "use". But it does not matter whether the other_module definition comes before the module definition since angular resolves this.

buunguyen commented 9 years ago

I hear you. I (and probably those submitting similar tickets) need correct order for dependencies because I define stuff in module A (e.g. BaseController) that is used in module B depending on A.

Per my question, the order works correctly if the check isDependecyUsedInAnyDeclaration is commented out. So the question is why having it in the first place? Is it an optimization?

simon04 commented 9 years ago

isDependecyUsedInAnyDeclaration has been added in the course of #7.

"I need correct order for dependencies": angular does not, and this gulp task does not provide it.

I created a small gist to show that a module A depending on B can be loaded before B (as your example where A and B are swapped): https://gist.github.com/simon04/031d602c550d31ea3778

buunguyen commented 9 years ago

@simon04 thanks for providing the gist.

If you add a global var like BaseController to B-define.js and use it from A-define.js, it wouldn't work because B-define.js comes before A-define.js. And that's the use case I'm after.

Regarding the linked issue #7, I suppose there's a way to fix the cyclic dep error while still ensuring dependency order. But I might be wrong.

joakimbeng commented 9 years ago

@buunguyen why do you use global variables? All best practices discourages that, and so does this plugin.