feat(core): Modify core module to implement style guidelines. #1202

rhutchison · 2016-02-11T05:23:09Z

Update the core module to implement the style guidelines.
Reduce size of init.js - moved filter logic out to it's own config.
Rename Menus to menuService

Ref: #657
Ref: #943

rhutchison · 2016-02-11T07:12:54Z

modules/core/client/app/init.js

    $locationProvider.html5Mode(true).hashPrefix('!');

    $httpProvider.interceptors.push('authInterceptor');
  }
-]);
-
-angular.module(ApplicationConfiguration.applicationModuleName).run(function ($rootScope, $state, Authentication) {


Moved to https://github.com/meanjs/mean/pull/1202/files#diff-4f015eca6b0b5f96c11d195d1a2058bc

trainerbill · 2016-02-11T16:58:54Z

@rhutchison Left some inline comments. Overall love the PR and is almost spot on with where I was going with it.

trainerbill · 2016-02-11T17:58:39Z

modules/core/client/app/config.js

@@ -1,23 +1,29 @@
-'use strict';
+(function (window) {


send in angular to be consistent

We don't send in angular anywhere, why here?

Just a thought since you are now sending window, may as well make it a true IIFE and add angular.

I disagree, as it's not part of the style guidelines. I can remove window if you wish.

Can we use $window instead?

$window should be used, no?

I don't think it can be, unless angular is injecting it into a controller/service.

lirantal · 2016-02-13T12:46:57Z

@rhutchison , @trainerbill once you guys wrap it up, then @codydaig or @ilanbiala can take a look and comment.

dprea · 2016-02-13T23:00:49Z

@rhutchison Based on a fork by @trainerbill I changed the auth-interceptor.service.js responseError function to:

function responseError(rejection) {
      if (!rejection.config.ignoreAuthModule) {
        switch (rejection.status) {
          case 401:
            var auth = $injector.get('Authentication');
            auth.signout();
            $injector.get('$state').transitionTo('authentication.signin');
            break;
          case 403:
            $injector.get('$state').transitionTo('forbidden');
            break;
        }
      }

rhutchison · 2016-02-15T22:12:23Z

@Zeldar that should probably go in a different PR. This is just to conform to the style-guide.

I think it is ready to go.

ilanbiala · 2016-02-16T01:28:20Z

modules/core/client/services/interceptors/auth-interceptor.client.service.js

+    }
+  }
+})();
+


ilanbiala · 2016-02-16T01:30:48Z

Added a few comments.

rhutchison · 2016-02-16T02:33:57Z

@ilanbiala took care of double IIFE - just waiting for successful build.

ilanbiala · 2016-02-16T02:50:35Z

@codydaig @lirantal 👀?

mg1075 · 2016-02-16T03:13:50Z

Please re-consider having everything use vm in the html views.

Yes, using vm consistently inside the controller function makes sense.
var vm = this;
Awesome.

However, I do not think it is advisable to have vm be the variable inside all the html views.
<div class="container" ng-controller="HeaderController as vm">

There is an important note in John Papa's guidelines about controllerAs with vm.
https://github.com/johnpapa/angular-styleguide#style-y032

Note: When working with larger codebases,using a more descriptive name can help ease cognitive overhead & searchability [emphasis added]. Avoid overly verbose names that are cumbersome to type.

<!-- avoid -->
<input ng-model="customerProductItemVm.text">

<!-- recommended -->
<input ng-model="productVm.id">

rhutchison · 2016-02-16T03:28:18Z

@mg1075 thank you for your feedback. I provided a commit for the community to review. If they are good with it, I will squash the commit.

@ilanbiala @lirantal @codydaig

rhutchison · 2016-02-16T04:12:51Z

My preference would be to stick with 'vm'.

@mleanos

I thought we were trying to get away from using the ng-controller attribute in the views.

I don't know if there is a way around it with HeaderController without implementing nested views. Any ideas?

The John Papa Styleguide examples aren't very clear on using the descriptive vm naming convention. What's the real benefit of doing this? There's only one controller per view, so it seems that vm would never have conflicts with any other vm.

I believe @mg1075 is saying for searchability in a project.

mleanos · 2016-02-16T04:37:56Z

@rhutchison I haven't tested this, or given it a thorough review. #1183

ilanbiala · 2016-03-19T17:58:34Z

modules/core/client/app/init.js

+  bootstrapConfig.$inject = ['$locationProvider', '$httpProvider'];
+
+  // Then define the init function for starting up the application
+  angular.element(document).ready(init);


Is angular.element($document).ready(init); valid?

Yes, but $document.ready also works I believe.

init.js:22 Uncaught ReferenceError: $document is not defined

did you inject it?

@rhutchison did you try injecting?

I have tried different ways, I don't think it's possible. I have not read or seen any examples that shows otherwise.

@ilanbiala @rhutchison If it's not possible (or easily possible) to use $document & $window in this case, then I say it's fine to leave it the way it is.

For the sake of not holding up this PR we can just focus on the style-guide implementation. However, @rhutchison if we're using window and/or document anywhere else let's change it to use $window/$document if it's an easy change. I haven't thoroughly checked for any other other usages.

ilanbiala · 2016-03-19T18:03:43Z

@rhutchison left some comments.

codydaig · 2016-03-19T20:48:14Z

I'd like to use $window and $document everywhere that it can be. Is there any case where we have to use window or document vs. $window and $document?

rhutchison · 2016-03-20T21:54:55Z

@ilanbiala @codydaig @mleanos @lirantal this is ready.

coveralls · 2016-03-20T22:05:08Z

Coverage remained the same at 70.615% when pulling 368bc79e8ecfda2bf8c0c86da0cd8bdb63e7d915 on Gym:core-styleguide into 89d302d on meanjs:master.

ilanbiala · 2016-03-21T14:39:13Z

modules/core/client/services/socket.io.client.service.js

-        this.socket.on(eventName, function (data) {
+    function on(eventName, callback) {
+      if (service.socket) {
+        service.socket.on(eventName, function (data) {
          $timeout(function () {


this can just be $timeout(callback(data)); no?

@rhutchison did this work?

I did not try, but it does not look like it would work.

ilanbiala · 2016-03-21T14:46:22Z

@rhutchison left some comments, but other than those that you need to fix the only thing I'm thinking of is the $window and $document stuff that @codydaig is also referencing. @rhutchison did you find any sources that say it isn't possible to use them in scenarios like ours? I would imagine Angular probably has a way of using $window or $document everywhere.

rhutchison · 2016-03-21T18:42:19Z

@ilanbiala within the context of angular, I'm sure it's possible, but not sure how it's done in these cases which are bootstrapping angular. Please let me know if you can find a way and I'll gladly update.

ilanbiala · 2016-03-22T00:23:51Z

modules/core/client/app/init.js

-      };
-    }
+    // Then init the app
+    angular.bootstrap(document, [app.applicationModuleName]);


https://docs.angularjs.org/api/ng/function/angular.bootstrap

Inject $document before Angular is even bootstrapped? ;-) Don't think so.

coveralls · 2016-03-23T00:22:19Z

Coverage remained the same at 70.615% when pulling 847ad7aea1613016f8fcd36346da59535b8755ee on Gym:core-styleguide into 59e6daa on meanjs:master.

mleanos · 2016-03-23T04:03:44Z

I was out of town over the weekend, so I apologize for the lack of feedback. I'm planning on pulling this down in the next day or so.

mleanos · 2016-03-23T05:08:24Z

@rhutchison I just pulled this down, and tested. Overall, it looks pretty good.

I think there are a few remaining comments that need to be addressed/finalized. Other than that this looks ready to me.

rhutchison · 2016-03-23T15:09:00Z

@mleanos thanks for reviewing. Please point me to the outstanding items.

codydaig · 2016-03-23T16:18:19Z

@rhutchison Looking at it closer, I see the aspects where you need to use window vs. $window. I'm cool with the way that it is currently. (I don't think I saw window or document being used inside a controller or service).

ilanbiala · 2016-03-23T17:41:12Z

LGTM, only change is the !== -1. Other than that, everything is fine.

Update the core module to implement the style guidelines. Reduce size of init.js - moved filter logic out to it's own config. Rename Menus to menuService

rhutchison · 2016-03-23T19:43:51Z

@ilanbiala done

coveralls · 2016-03-23T19:54:08Z

Coverage remained the same at 70.615% when pulling b2462ec on Gym:core-styleguide into 59e6daa on meanjs:master.

ilanbiala · 2016-03-23T22:52:31Z

@mleanos LGTM.

codydaig · 2016-03-24T02:44:01Z

LGTM. @mleanos Merge before merge conflicts arise!!!!

rhutchison force-pushed the core-styleguide branch 2 times, most recently from 8656e35 to 6f97a0a Compare February 11, 2016 06:17

rhutchison reviewed Feb 11, 2016
View reviewed changes

trainerbill reviewed Feb 11, 2016
View reviewed changes

dprea mentioned this pull request Feb 13, 2016

[UNRESOLVED] JWT Authentication #1163

Closed

rhutchison force-pushed the core-styleguide branch 2 times, most recently from 8360603 to 691aa0b Compare February 14, 2016 02:37

ilanbiala reviewed Feb 16, 2016
View reviewed changes

modules/core/client/services/interceptors/auth-interceptor.client.service.js

}

}

})();

Copy link

Member

ilanbiala Feb 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline

rhutchison force-pushed the core-styleguide branch 2 times, most recently from 4369199 to 2f9fefb Compare February 16, 2016 02:32

ilanbiala added the enhance:feature label Feb 16, 2016

ilanbiala added this to the 0.5.0 milestone Feb 16, 2016

ilanbiala reviewed Mar 19, 2016
View reviewed changes

rhutchison force-pushed the core-styleguide branch from 4db1e19 to 368bc79 Compare March 20, 2016 21:52

ilanbiala reviewed Mar 21, 2016
View reviewed changes

ilanbiala reviewed Mar 22, 2016
View reviewed changes

rhutchison force-pushed the core-styleguide branch from 368bc79 to 847ad7a Compare March 23, 2016 00:07

lirantal mentioned this pull request Mar 23, 2016

Roadmap planning for MEAN.JS 0.5.0 #657

Closed

8 tasks

feat(core): Modify core module to implement style guidelines.

b2462ec

Update the core module to implement the style guidelines. Reduce size of init.js - moved filter logic out to it's own config. Rename Menus to menuService

rhutchison force-pushed the core-styleguide branch from 847ad7a to b2462ec Compare March 23, 2016 19:42

mleanos merged commit dde9682 into meanjs:master Mar 24, 2016

feat(core): Modify core module to implement style guidelines. #1202

feat(core): Modify core module to implement style guidelines. #1202

Conversation

rhutchison commented Feb 11, 2016

Choose a reason for hiding this comment

trainerbill commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lirantal commented Feb 13, 2016

dprea commented Feb 13, 2016

rhutchison commented Feb 15, 2016

Choose a reason for hiding this comment

ilanbiala commented Feb 16, 2016

rhutchison commented Feb 16, 2016

ilanbiala commented Feb 16, 2016

mg1075 commented Feb 16, 2016

rhutchison commented Feb 16, 2016

rhutchison commented Feb 16, 2016

mleanos commented Feb 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanbiala commented Mar 19, 2016

codydaig commented Mar 19, 2016

rhutchison commented Mar 20, 2016

coveralls commented Mar 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanbiala commented Mar 21, 2016

rhutchison commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 23, 2016

mleanos commented Mar 23, 2016

mleanos commented Mar 23, 2016

rhutchison commented Mar 23, 2016

codydaig commented Mar 23, 2016

ilanbiala commented Mar 23, 2016

rhutchison commented Mar 23, 2016

coveralls commented Mar 23, 2016

ilanbiala commented Mar 23, 2016

codydaig commented Mar 24, 2016