Article Refactor

[trainerbill]

Superceeds #800

@ilanbiala @rhutchison

[ilanbiala]

Shouldn't this be 'ArticlesCreateController' or something similar since we are doing that above?

[trainerbill]

Well previously I had them all separate controllers but that got shot down. I can see the need for a create controller if we don't want to pass in a blank article to resolve. Then the ArticlesController would handle edit/view since it needs an article to resolve? @rhutchison

[trainerbill]

@rhutchison Angular Stylized

I will fix the tests once we finalize the code. Do the tests need to follow any sort of guidelines?

[ilanbiala]

I'm not a fan of the IIFEs. Is anyone of the belief that what we have inside is actually being initialized as a global variable and should definitely not be there. It seems unnecessary to me.

[trainerbill]

@ilanbiala I think the point is to have a style guide and stick to it everywhere, whether or not it is functionally necessary.

[codydaig]

I would second the IIDEs. Other than that, I've been following th John Papa style guide in my most recent project and love it.

[ilanbiala]

@codydaig what benefit does the IIFE offer? I don't see us creating many global variables.

[trainerbill]

@rhutchison @ilanbiala @lirantal @codydaig Can I get a code review here? I fixed the tests and everything is looking pretty good to me. thx

As for the IIFEs, I think it is best practice and it is in line with our style guide, regardless if it is needed functionally.

[simison]

I don't see us creating many global variables.

Other projects might, as it's a boilerplate. It's a good practise to pass on for others, especially for less experienced developers.

Although for articles.client.module.js it indeed looks a bit silly. :-)

[ilanbiala]

@simison the other thing is that I'm pretty sure these IIFes are set up incorrectly. The whole point is that there are 0 global variables. In articles.client.module.js, ApplicationConfiguration isn't passed in like it should be through the IIFE, but it is used as a global, so I'm not even sure why we are just adding IIFEs to add them. I'm still not in support of it, but if we are going to do it at all, then it should be implemented such that there are no global variables.

[mleanos]

Other projects might, as it's a boilerplate. It's a good practise to pass on for others, especially for less experienced developers.

@simison With the architecture of this boilerplate, I don't see the need for IIFE's. As long as users of this project keep their project's architecture inline with this implementation, then there shouldn't be a need for us to introduce this particular best practice. I would even say it would be confusing to less experienced developers; whereas how it's setup now, it's pretty easy to understand & read the code.

If someone deviates from the current implementation, and finds that they need to implement this best practice, then they could do so. But it would be a conscious effort on their part, and they'd have a specific reason for doing so.

[simison]

@ilanbiala @mleanos All valid points and good food for thought!

[trainerbill]

@ilanbiala You are correct on the articles.client.module.js and I will change that. If you find any others let me know.

I don't think we should deviate from a style guide that we chose to use... I think we need to stick to it otherwise how is a dev gonna know what to do and what not to do?

[trainerbill]

Would we be ok with angular being global? Or are we passing that into the IIFE as well?

[codydaig]

@ilanbiala I meant I agree with you. I don't like IIFEs at all. I dislike how your code starts indented vefore you even write your first line. As long as you concat before you minify and uglify, it doesn't make a difference.

[codydaig]

Sounds like its been decided to not use IIFEs. @trainerbill go ahead and remove them. I'll be able to do a more through review later day as well.

[trainerbill]

@codydaig I don't know why we would decide that. If we are going to have a style guide we should really stick to it. Picking and choosing leaves gray areas. Where was the discussion and decision made? I know of 3 people that support them.

[codydaig]

#636

[ilanbiala]

@trainerbill we are picking a style guide and we are doing what makes sense for the framework. A style guide is exactly that, a guide for what you should do. It isn't a rulebook that says do everything exactly as I say. IIFEs aren't necessary because of the way our app is set up, therefore why add the complication and extra code that does nothing.

[trainerbill]

@ilanbiala fair enough. Someone should make a MEAN.js styleguide then. It makes it easier as a contributor to know up front what I need to do, rather than go through 50 code reviews for style issues.

[ilanbiala]

What is vm?

[codydaig]

view model. It's supposed to replace $scope.

[simison]

@ilanbiala https://github.com/johnpapa/angular-styleguide#controlleras-with-vm

[ilanbiala]

The only issue is we can't just change it in one place. Maybe we do that as migration to 1.5?

[mg1075]

@mleanos - I think there would be overhead if controllerAs is not used in all modules.

If controllerAs is only used in one or two modules to start off, now a person has to start switching between patterns: this view and controller use controllerAs, this view and controller don't; this view and controller follow the angular style guidelines, this other view and controller don't.

One advantage of using controllerAs noted in the guidelines is:

Why?: Helps avoid using $parent calls in Views with nested controllers.

Imagine I have an app where there is an angular Controller that is a parent controller, sort of like a shell controller for an app. And then, of course, I have a controller nested inside. Using controllerAs means I won't have to use $parent, and in a framework like MEAN.JS where - in my experience - you have to jump around from place to find things, avoiding the use of $parent will make the code easier to read. But this is unlikely to happen unless core and users use controllerAs.

[mleanos]

@mg1075 I'm advocating to use controllerAs. But I would rather avoid tasking one contributor with implementing this style into every module in one PR. I would rather see this done by multiple contributors, in a shared branch; which is what I view master at this point, since our next release is a major bump to 0.5.0.

IMO, most users of this framework don't modify the core, or users module. They take our example CRUD (Articles) and use it as a guide for developing their own custom modules. This is another reason why it makes more sense to implement the style-guide into the Article module to start.

[mleanos]

@mg1075 I'm on Gitter right now, if you'd like to discuss this there. To avoid cluttering this issue.

[codydaig]

@mleanos @mg1075 @lirantal @ilanbiala I'd much rather see it broken up by module rather than one large PR. Because that PR is going to be a nightmare for anyone doing it if any changes are made along the way. All modules need to be switched over before 0.5 is released though to avoid overhead. The master branch is considered development, so anyone using it in production runs the risk of some type of overhead as not all proposed changes will have been made yet.

[ilanbiala]

@codydaig do you want to oversee that then? It's fine if people submit PRs too, but if not then would you be able to refactor that code? I'll gladly review it as well to help out.

Also, one thing that I'd want to see if we're doing it now is avoiding $scope as much as possible. https://toddmotto.com/no-scope-soup-bind-to-controller-angularjs/ http://blog.ninja-squad.com/2015/07/21/what-is-new-angularjs-1.4/ have suggestions on how to do that, but they require Angular 1.4.

[codydaig]

@ilanbiala I can oversee that. I've worked with the John Papa style guide. (Actually more familiar with that then using $scope.) I agree, $scope shouldn't have to be used anywhere with the ControllerAs syntax. I will try upgrading Angular and see if it breaks anything. These next 2 weeks are crazy for me, but come January I should be able to crank these out.

[mg1075]

@codydaig - recall @rhutchison comments in gitter about using controllerAs:

it seems to be working well, I am still adopting it in another project
we change to best-practice as we touch files
only thing that sucks is though may not be able to remove $scope from DI
for things like $scope.$apply(), $scope.$emit(), $scope.$watch, etc.
but controllerAs is working well

...and...

one thing I came across today, is in instances like a menu or tab system, where I use
templateProvider/controllerProvider on my routes to dynamically set my controller/templates, there
is no controllerAsProvider, so I need to define my controller like so HomeController as vm
example

function controllerProvider($stateParams, companyTabs) {
    if (companyTabs[$stateParams.tab].controller) {
        var controller = companyTabs[$stateParams.tab].controller.split(' as ');

        if (companyTabs[$stateParams.tab].controllerAs) {
            controller[1] = companyTabs[$stateParams.tab].controllerAs;
        }

       return controller.join(' as ');
    }
}],

[mleanos]

I'd like to undertake the refactoring of the Users Admin feature. I have a good amount of experience with John Papa's styleguide as well.

As for the use of $scope, we don't need it in any controllers. For anything that needs to use it like $broadcast, $emit, etc., then that functionaly should be abstracted out into services, and/or directives.

[mleanos]

@trainerbill @rhutchison what's the plan here?

[rhutchison]

tested and looking good

@trainerbill clean up the commit message and squash the commits - LGTM

[mleanos]

@rhutchison Why is this included? It's already in master.

We should be using rebase, not merges right?

[ilanbiala]

Why do this? Why not just do it in the controller

[trainerbill]

@ilanbiala It makes the controller smaller and allows the use of UI-Router nested views to share the resolve between the views and other controllers.

[mleanos]

@trainerbill There's a reference to a merge conflict still in the code.. I commented on the line that has <<<<<<<< HEAD ...

Also, there are some older line comments that haven't been addressed.