Manageiq: [WIP] JS style guidelines (draft)

[himdel]

(This is a draft of a future docs document detailing our Javascript style guidelines for ManageIQ and SSUI.)

formatting

indentation

Use 2 spaces per indent level, not 4, nor tabs..

// good
function foo() {
  var x;
  if (x === undefined) {
    console.log('Whee!');
  }
}

// bad
function foo() {
    var x;
    if (x === undefined) {
    console.log("Unwhee");
    }
}

object and array literals

// good
{
  foo: bar,
  baz: quux,
}
[
  foo,
  bar,
  baz,
]

// good (for small objects/arrays)
{ foo: bar, baz: quux }
[ 1, 2, 3 ]

// ok (makes more sense for small values, don't use with anything longer)
[1, 2, 3]

// discouraged
{foo: bar}
[1,2,3,]

trailing commas

Please prefer trailing commas in multiline object or array literals, they are fully supported now and make nicer diffs..

//good
[
  1,
  2,
  3,
]

// discouraged
{
  foo: bar,
  baz: quux
}

// weird
[ 1, 2, 3, ]

function calls

No spaces surrounding the brackets, split long lines after { or [. If you're splitting the call into multiple lines close the parenthesis right after the last argument (same line), and try to keep each argument on a separate line.

// good
foo(1, 2, 3);
bar(foo, {
  a: b,
  c: d,
});

// ok for lots of arguments
foo(1,
    2,
    3)

// bad
quux( { many: many, keys: and, values: true } );

chained methods (esp. then/catch/done/success/etc.)

Please always split chained method calls into multiple lines, starting continuing lines with the dot, and use one indent level for these lines.

// good
$http.get('/foo')
  .then(callback)
  .catch(errback);

var sorted = array
  .map(func)
  .filter(fn)
  .sort();

// discouraged
$http.get('/foo').then(callback).catch(errback);
array.map(func).filter(fn).sort();

if/while blocks

Please use a space before the first parenthesis, use egyptian brackets, and prefer brackets even for single-statement bodies.

// good
if (expression) {
  body;
} else {
  body;
}

// discouraged
if(expression)
  bleh();

expressions

Always surround binary operators with spaces.

// good
(5 + 4 * (3 + 2)) > 7

// bad
5+4*3

switch statements

Unlike in ruby, indent case one indent level more than switch. Also, please explicitly mark case-to-case fall-through with // pass.

// good
switch (x) {
  case 1:
    whatever;
    break;

  case 2:
  case 3:
    whatever;
    // pass
  case 4:
    whatever;
    break;

  default:
    whatever;
}

// bad - case indented badly
switch (x) {
case 5:
  foo;
}

// bad - "hidden" fall-through
switch (x) {
  case 1:
    do_something;
  case 2:
    probably_a_bug;
}

naming

functions

Use camelCase for function/method names.

// good
function miqOnLoad(foo) {...}

// bad
function add_flash(msg) {...}

angular

Prefer full FooController for controllers, FooService for services, FooFactory for factories, foo for directives and foo or foo.bar for modules.

globals

don't

Don't use global global variables, ever.

Also, please try not to create them accidentally by omitting var.

// good
function foo() {
  var bar = 5;
}

// good if you really need a static variable (and don't access it from elsewhere)
function bar() {
  bar.baz = bar.baz || 0;
  bar.baz++;
}

// bad
function foo() {
  bar = 5;
}

// still bad
var whatever = false;
function bar() {
  whatever = true;
}

ManageIQ global object

If you really have to keep global state, add it to the ManageIQ global object, defined in miq_global.js, and add a comment with a purpose.

But please think twice before introducing a new one.

general

comparison operators

Please prefer === and !== to == and !=, but only when it makes sense.

// good
if (x === "5") {...}

// ok, if really checking for undefined or null
if (null_or_undefined == null) { ... }

// usually good enough
if (truthy) { ... }

// discouraged (array.length by itself is perfectly fine there)
if (array.length === 0) { ... }

quoting

Prefer single quotes, unless really inconvenient.

// good
var str = 'We are the Borg';

// bad (be consistent!)
var str = "We" + 'are' + "the" + 'Borg';

// perfectly fine
var str = "I don't want to escape all the apostrophes.";

es6

We do support ES6 now (or a subset thereof - via babel 5) provided the file has an .es6 extension. Please use your judgement and consult config/initializers/sprockets-es6.rb. Also, we support all ES6 library methods (including Promise) in .js files via es6-shim.

/// in .js files..
// bad
const foo = 5;

// ok
var foo = Promise.all([
  $http.get(...),
  $http.get(...),
]);

/// in .es6 files
// ok
const { foo, bar } = { foo: 5, bar: 6, quux: 7 };

// bad
import { foo } from "baar.js";

linting

In manageiq-ui-self_service, you can run gulp vet, which runs jshint and jscs - but the configuration is currently _too_ opinionated. (TODO)

In manageiq, you can run linters manually, as soon as we add their config files.. (TODO)

For anything not specified here, please consult https://github.com/airbnb/javascript .

[himdel]

@martinpovolny , @romanblanco do you have anything to add/comments/..?

[himdel]

TODO:
[ 1, 2, 3 ] vs [1, 2, 3] ... [1, 2, 3] definitely makes sense for small stuff.. should make it clear that that is ok..

[martinpovolny]

@abonas, @skateman, @AparnaKarve : comments?

@himdel, @skateman: how many of these rules can we get checked on each commit?

[himdel]

@martinpovolny almost all of them, just a bit of a pain to configure properly..

[AparnaKarve]

@himdel Why is this discouraged?

// discouraged
{
  foo: bar,
  baz: quux
}

[romanblanco]

@AparnaKarve missing coma after quux I guess:

// wrong
{
  foo: bar,
  baz: quux
}

// right
{
  foo: bar,
  baz: quux,
}

[AparnaKarve]

@romanblanco A comma even for the last item? Somehow that does not sound right.

[himdel]

Exactly that :) .. because then you add a new line and the diff can look either like this...

 {
   foo: bar,
-  baz: quux
+  baz: quux,
+  something: new
 }

.. or like this ..

 {
   foo: bar,
   baz: quux,
+  something: new,
 }

Also.. https://github.com/airbnb/javascript#commas--dangling and http://www.2ality.com/2013/07/trailing-commas.html ;)

[skateman]

yay, weird :lollipop:

[abonas]

@romanblanco A comma even for the last item? Somehow that does not sound right.

+1 this is weird. it is even more weird to follow it only in js code.

[skateman]

@abonas the main advantage is that it can avoid merge conflicts

[abonas]

@abonas the main advantage is that it can avoid merge conflicts

I understand but it's not really standard, and the ruby code doesn't follow it, so it can just create headache for developers due to inconsistency.

[himdel]

I understand but it's not really standard

On the contrary, it _is_ recommended by multiple JS standard guides, including the aforementioned Airbnb guide.. John Papa's Angular Style Guide also mentions disallowTrailingComma: null, ..

and the ruby code doesn't follow it

We definitely can't use the preferred Ruby style, that's simply not done in the JS world.. Besides, it's not quite true, there are 1700 (!) occurences of the trailing comma in our Ruby code.. ( ag ',\s*$\s*[\}\)\]]' $(find -name \*.rb) | nl )

[abonas]

Not sure I understand - is this for manual checking or there will be some kind of automatic validation of those rules?

[himdel]

@abonas miq-bot will do the checking, the same way as for ruby

[abonas]

@abonas miq-bot will do the checking, the same way as for ruby

@himdel :smile: but with which validation tool? (currently there's Rubocop and haml-lint validations, but they are not for js)
cc @zeari - you might need to add something to murphy.sh soon :smile:

[himdel]

@abonas that part I'm still working on.. It will be one or more of eslint, jscs, and jshint.

[zeari]

cc @zeari - you might need to add something to murphy.sh soon

[himdel]

@zeari What's murphy.sh?

[zeari]

@zeari What's murphy.sh?

Its does a rubocop check but just on lines in files added on the last commit. Same as miq-bot but locally on your own machine.
https://github.com/zeari/miq-helpers/blob/master/murphy.sh

[himdel]

Right, nice .. could be even useful to put it in the miq repo..

I'll point you to the relevant miq-bot patches when they exist, so that you can do the same thing..

[zeari]

Right, nice .. could be even useful to put it in the miq repo..

I'll point you to the relevant miq-bot patches when they exist, so that you can do the same thing..

great, thanks.

[jkremser]

+1 on doing as much as possible the same way as https://github.com/airbnb/javascript

[karelhala]

What about single and double quotes? Sometimes I stumble across file where there's both of them used and it does not look good. We should agree on one or another and force to use it. I'd prefer single quotes since we were using them in hawkular.

[himdel]

I'd rather not force single vs double, there are uses for both.. Especially in our code where we still manipulate the dom, etc... We could do it for specific purposes, like force double quotes in object literals if you need any, or things like that, but I'm not sure we can actually check those without checking the rest..

[skateman]

I'd prefer single quotes, also in our rubocop config

[romanblanco]

+1 for single quotes

[skateman]

Chained methods

Probably arranging the dots?

$http.get('/foo')
     .then(callback)
     .catch(errback);

If blocks

What about short one liners?

if (something) do_awesome_stuff();

[jkremser]

single x double quotes: http://stackoverflow.com/a/31883471/1594980 choose your side :)

wrt ES6: what's the strategy? Starting new js files in ES6 or even rewriting the existing ones. Or just a PoC and do not touch it :] What about the fat-arrows instead of the clumsy function(){}, the fat-arrows would make the d3 code more readable, because it's full of those .attr('foo', function(e){return e.bar;})

[himdel]

OK, OK, I'll add a "prefer single quotes" bit.. with the exception of whenever you need to use an apostrophe inside.. :)

[skateman]

:+1:

[himdel]

Chained methods.. honestly I'm not sure which is the right one, let's look at the options..

same indent as the line before

$http.get('/foo')
.then(function(bar) {
  baz();
});

I like this one because it makes the blocks correctly indented, as in, you get the same indent for a callback to the first line, as for the second one. But starting with a dot and unindented does look a bit weird...

one indent to the right

$http.get('/foo')
  .then(function(bar) {
    baz();
  });

Can work, but it does have a bit confusing indent .. as in, any statements after that will look like they're one level to the left than they should be.

dot under dot

$http.get('/foo')
     .then(callback)
     .catch(errback);

Looks pretty, but makes it pretty much impossible to pass function literals as arguments, because that indent will be too much. And it's unclear what to do when the first line has no dots, and I'd prefer to _always_ put .then on the next line..

Thoughts anyone?

[himdel]

if (something) do_awesome_stuff();

let's not :) Like.. if we do check indent levels, we _can_ safely allow..

if (something)
  do_awesome_stuff();

But putting it on the same line just feels wrong..

[karelhala]

What about short one liners?
if (something) do_awesome_stuff();

I don't like one liner if statements, much cleaner way is to use && operand

something && do_awesome_stuff();

[himdel]

wrt ES6: what's the strategy? (@Jiri-Kremser)

Right now, feel free to start new files in ES6, but we still have that .js vs .es6 distinction, until we update the tooling accordingly.. I'm definitely pro-fat-arrows, but, only in .es6 files..

[jkremser]

@himdel do I get it right that the .es6 files will be transpiled to the .js files that are ES5 compliant because of compatibility reasons? If that's the case, we would need to support also the source maps to be able to debug the original code in the browser dev tools. IIRC, @mtho11 was doing this for TypeScript in Hawkular, so perhaps he can give a hand with it.

[himdel]

@Jiri-Kremser yes, you get that right. But.. it's pointless to spend time on that _now_. We need to revamp the entire tooling first, both to actually support babel6, and to be able to generate source maps, etc. But it's definitely on the list :)

[karelhala]

There's more to chain methods, what about chain methods with assign (or as return statement)

var a = _.chain(users)
         .sortBy('age')
         .map(function(o) {
           return o.user + ' is ' + o.age;
         })
         .head()
         .value();

or as used in lodash documentation

var a = _
  .chain(users)
  .sortBy('age')
  .map(function(o) {
    return o.user + ' is ' + o.age;
  })
  .head()
  .value();

I like one indent (same as airbnb) and variable assign same to lodash's way.

[romanblanco]

@karelhala @himdel @skateman

What about short one liners?
if (something) do_awesome_stuff();

I don't like one liner if statements, much cleaner way is to use && operand

something && do_awesome_stuff();

&& operand is better that one liner if, but anyway, using multi-lined if seams as a best readable to me.

if (something) {
    do_awesome_stuff();
}

I also prefer having the end bracket } instead of saving one line, as it clearly tells where the if ends.

[himdel]

Updated the text to clarify multiline function calls, and also changed the chained methods section to use 1 indent level, and show a variable assignment as well...

[himdel]

Regarding the actual linter, given the recent news that ESLint and JSCS are in the process of merging (into ESLint), ESLint becomes a clear winner, so it will be the main tool..

We _may_ still need to also use jscs 3 or jshint for some rules, but.. maybe not :)

[himdel]

Added a WIP PR with an eslint config based on this issue (and a bunch of related cleanups) .. https://github.com/ManageIQ/manageiq/pull/9019

[mtho11]

We also need an Angular Style guide as well; something like:

• https://github.com/mgechev/angularjs-style-guide
• https://toddmotto.com/opinionated-angular-js-styleguide-for-teams/
• https://github.com/johnpapa/angular-styleguide/blob/master/a1/README.md
• https://google.github.io/styleguide/angularjs-google-style.html

And we can just fork it to our needs. Should probably be a separate PR to review.

[AllenBW]

:metal: to all the above!! working on implementing these rules and more in the SSUI, progress visible here: https://github.com/ManageIQ/manageiq-ui-self_service/issues/121

eslintrc.json for this work is default eslint config follows:

"env": { "browser": true, "amd": true, "jquery": true, "es6": true }, "extends": [ "eslint:recommended", "angular" ],

[AllenBW]

Oh my, hasn't been touched in almost a year!! ! 😲 Aside from the no comma dangle, anyone have major issues with https://standardjs.com/ ? for the lazy

@chriskacerguis

Edit: and by issues, I mean problem with say the sui using standardjs, at the cost of any unwilling individual's efforts (unless ya want to 🌮 🍮 💃 )

[skateman]

@AllenBW does standardjs :heart: typescript?

[himdel]

@AllenBW well, standard js is just eslint under the hood, but the configuration.. Some parts are quite different from ours (no semicolons, space after function name), while the rest we already have in our eslint config.

So... what's the benefit? And is it really that much better than eslint to justify removing all semicolons? :)

[chriskacerguis]

I think that the biggest benefit is what Standard says. No configuration, no maintaining various linting files. We don’t need to keep those files in each repo and keep them in sync...which I think is a big plus.

[AllenBW]

@skateman funny you should ask.... https://standardjs.com/#can-i-use-a-javascript-language-variant-like-flow-or-typescript (there's always a plugin for that) 🤣 😍 🤧

@himdel Agreed, some parts are quite different and with standard you can't really turn stuff off or on, its kinda THIS IS THE WAY so that's kinda a con... kinda...

Benefits are:

1. widely adopted, modern approach to syntax - its what all the cool kids are doing, but seriously, using a convention that is in line with bunches of other software is a good idea unless we have to be different for some reason
2. less maintenance - no more maintaining that eslintrc.json as we begin using modern js, ECMAScript 6/7/8 older rules make more headache than they save (conflict resolution between conflicting eslint rules)
3. saves time - no debate, just standard

[martinpovolny]

What is the effort to reformat our sources to the "Standard"? Is there a tool to do that for us?

[chriskacerguis]

@martinpovolny Minimal effort...just need to run:

standard --fix

[mtho11]

@AllenBW a PR to see how this would work can go a long way ;)

[AllenBW]

🤔 @mtho11 see how what would work?

[himdel]

:+1: And if you're doing a PR, please be aware of these in ui-classic:

• app/javascript/ is the only place where ES6/Typescript is alllowed (well, except for config/webpack)
• app/assets/javascripts/ needs to be ES5-only
• spec/javascripts/ needs to be es5 only, and use a slightly different ruleset from app - for example, using $scope.$digest() is usually a sign of a problem in controllers, whereas in specs, it's just another day; also, there's a list of 10 globals which are completely valid in specs and nowhere else (it, describe, context, ...)
• app/assets/javascripts/{controllers,services,components,directives}/ need to use the same rules as app/assets/javascripts/, but also include angular-related checks, which can't be used for the rest (eg. "Use $timeout instead of setTimeout" is something you really want to be warned about in angular code, but is completely wrong outside angular)
• we do need to use the linter rules from eslint-plugin-angular
• we may need some custom rules

If some of those seem a bit hard without a config file, we're in agreement ;D

[mtho11]

@AllenBW how standard.js would work with typescript plugin.

[mtho11]

Another plus for tslint is that angular2+ CLI autogenerates and uses it.
And http://codelyzer.com (also in CLI) uses tslint.

[mtho11]

And IDEs like webstorm support it highlighting all rule violations inline with the code. Here is video showing the integration: https://blog.jetbrains.com/webstorm/2015/11/webstorm-11-released/

[AllenBW]

Oh apologies for the murky sentiment, @mtho11 I'm proposing this for the https://github.com/ManageIQ/manageiq-ui-service

discussion is here cuz its the only place code style has been discussed, as mentioned above, standard does support plugins, and there are typescript plugins

@himdel duly noted!! saving this, just cuz its valuable info about classic 😍

[mtho11]

@AllenBW thanks for the clarification 😺

[martinpovolny]

Oh apologies for the murky sentiment, @mtho11 I'm proposing this for the https://github.com/ManageIQ/manageiq-ui-service

Oh well, then I have no objections.

In the OPS UI we might need to take a more step by step approach as usual.

[AllenBW]

FYSA for anyone who likes to 👓 https://github.com/ManageIQ/manageiq-ui-service/pull/886 🍷 🤽‍♂️

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

[JPrause]

@himdel is this issue still relevant. If not, please close. Otherwise, please remove the "stale" label.

[himdel]

I think the discussion is still relevant, even though the rules mentioned above were already converted to the various .eslintrc.json files.

Adding pinned instead.

[himdel]

Oh, except this is still in the core repo.

In that case..

@miq-bot move_issue ManageIQ/manageiq-ui-classic

[miq-bot]

This issue has been moved to https://github.com/ManageIQ/manageiq-ui-classic/issues/5045