klei / gulp-angular-filesort

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

Wrong sorting with files in deep directory structure #12

Open jrmichael opened 9 years ago

jrmichael commented 9 years ago

I have a directory structure similar to the following:

app
  \-main
    \-app.js
  \-admin
    \-item
      \-itemController.js

app.js has a module deifined angular.module('app', []) itemController.js adds a controller to app module angular.module('app').controller(...)

Sorting result puts itemController.js before app.js. When I tried putting 'itemController.js' one level higher (in admin directory) sorting worked as expected.

bholben commented 9 years ago

I'm seeing the same behavior. My directory structure looks like this:

build/
  components/
      main/
          main-controller.js
          main-directives.js
  app-controller.js
  app.js
  index.html

After injecting with gulp-angular-filesort, the app.js file is below the main-*.js files. It needs to be before. The app-controller.js file gets sorted correctly (after app.js), but the *.js files further down the directory tree don't get the same treatment.

bholben commented 9 years ago

I've been testing some more with dependencies that should be ordered like this: app.js --> app-factory.js --> app-controller.js. When I delete the components directory and keep things flat, it sorts perfectly. If I move the factory and controller file into another directory, it sorts improperly. If I keep it flat and rename the app-factory.js to bapp-factory.js, it sorts improperly.

bholben commented 9 years ago

After looking at this further, I've had success with gulp-angular-filesort when each file has a unique module name and the setter syntax, i.e. angular.module('myModule', []) - no matter whether nested in a directory structure or not. I experience this problem when using the getter syntax - angular.module('myModule') in separate files as @jrmichael shows in his issue description.

My understanding is that getter syntax is a valid approach, however this will not work with gulp-angular-filesort. If my assessment is correct, then I'd recommend to edit the README and advise users that they need to have unique module names and setter syntax in each file.

joakimbeng commented 9 years ago

Hmm, it looks like a bug to me. This is not an intended behavior.

bholben commented 9 years ago

I'm forming the opinion that the order does not matter for custom scripts. I just read this article and then changed the order around myself with no problem.

It may be important to load 'jquery.js' before 'angular.js' before custom scripts, but it appears that the custom script order does not matter with the way Angular dependency injection works. Am I wrong here? Perhaps my issues were from something else.

jrmichael commented 9 years ago

I found the problem. I was not reading files before piping them to angularFileSort gulp.src(['./web/main/**/*.js'], {read: false}).pipe(angularFilesort()) As a result ng-dependencies was not analyzing their contents. I was lucky enough to have the results in a proper order because there were not that many of them. I will create a pull request with a check that will help others avoid this confusion in future.

dmitriykharchenko commented 9 years ago

I think files order should be consistent from build to build, otherwise, for example, there is no profit from using gulp-rev. In my project I made 2 builds in different time, without any changes, but got different files: app-47831b04.js and app-3ffdff5f.js.

jrmichael commented 9 years ago

@aki-russia The initial problem here was using read flag with false value. The resulting order was driven by gulp.src(...) I think you should create a new issue for this.

@joakimbeng Can you close #12 ? We already found a solution and you even merged a PR.

forty8bits commented 9 years ago

I'm seeing incorrect sort ordering on a project which roughly follows the guidelines at https://github.com/johnpapa/angular-styleguide. Am I right in thinking there are no workarounds currently? Creating a new, unique module using setter syntax for every file couldn't be a realistic solution for most projects following this kind of structure.