[WIP] JS style guidelines (draft) #8781

himdel · 2016-05-18T14:02:37Z

(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 .

The text was updated successfully, but these errors were encountered:

himdel · 2016-05-19T09:45:22Z

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

himdel · 2016-05-19T14:57:15Z

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 · 2016-05-19T15:56:06Z

@abonas, @skateman, @AparnaKarve : comments?

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

himdel · 2016-05-19T17:07:16Z

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

AparnaKarve · 2016-05-19T17:18:30Z

@himdel Why is this discouraged?

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

romanblanco · 2016-05-19T19:41:46Z

@AparnaKarve missing coma after quux I guess:

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

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

AparnaKarve · 2016-05-19T23:27:45Z

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

himdel · 2016-05-20T08:27:18Z

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 · 2016-05-20T08:28:32Z

yay, weird 🍭

abonas · 2016-05-22T15:28:45Z

@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 · 2016-05-22T15:38:42Z

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

abonas · 2016-05-22T15:57:40Z

@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 · 2016-05-22T18:01:33Z

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 · 2016-05-23T12:27:58Z

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

himdel · 2016-05-23T12:39:55Z

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

abonas · 2016-05-23T12:47:38Z

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

@himdel 😄 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 😄

himdel · 2016-05-23T12:51:55Z

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

zeari · 2016-05-23T12:52:27Z

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

himdel · 2016-05-23T12:53:52Z

@zeari What's murphy.sh?

zeari · 2016-05-23T12:57:00Z

@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 · 2016-05-23T12:59:42Z

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 · 2016-05-23T13:07:54Z

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 · 2016-05-23T16:22:13Z

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

karelhala · 2016-05-25T08:59:13Z

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 · 2016-05-25T09:03:04Z

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 · 2016-05-25T09:04:03Z

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

romanblanco · 2016-05-25T09:04:16Z

+1 for single quotes

skateman · 2016-05-25T09:12:30Z

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();

AllenBW · 2017-08-24T12:54:38Z

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 · 2017-08-24T12:57:09Z

@AllenBW does standardjs ❤️ typescript?

himdel · 2017-08-24T13:11:09Z

@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 · 2017-08-24T13:18:58Z

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 · 2017-08-24T14:12:39Z

@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:

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
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)
saves time - no debate, just standard

martinpovolny · 2017-08-24T14:51:21Z

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

chriskacerguis · 2017-08-24T14:52:38Z

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

standard --fix

mtho11 · 2017-08-24T16:36:38Z

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

AllenBW · 2017-08-24T16:44:09Z

🤔 @mtho11 see how what would work?

himdel · 2017-08-24T16:46:04Z

👍 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 · 2017-08-24T16:50:06Z

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

mtho11 · 2017-08-24T16:59:38Z

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

mtho11 · 2017-08-24T17:06:30Z

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 · 2017-08-24T17:52:07Z

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 · 2017-08-24T18:02:29Z

@AllenBW thanks for the clarification 😺

martinpovolny · 2017-08-25T05:24:24Z

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 · 2017-08-25T16:28:46Z

FYSA for anyone who likes to 👓 ManageIQ/manageiq-ui-service#886 🍷 🤽‍♂️

miq-bot · 2018-02-26T05:00:52Z

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 · 2018-12-05T13:41:54Z

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

himdel · 2018-12-05T13:57:44Z

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 · 2018-12-05T13:59:46Z

Oh, except this is still in the core repo.

In that case..

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

miq-bot · 2018-12-05T14:00:19Z

This issue has been moved to ManageIQ/manageiq-ui-classic#5045

himdel mentioned this issue May 19, 2016

Fix spacing issues (4->2) in Middleware Topology js ctrl spec #8653

Merged

chessbyte assigned himdel May 19, 2016

AllenBW mentioned this issue Sep 14, 2016

ESLint: Globally ignores "__" and "N_", fixes indents/formatting ManageIQ/manageiq-ui-service#132

Closed

himdel mentioned this issue Jun 27, 2017

Disable some Code Climate warnings ManageIQ/manageiq-ui-classic#1593

Merged

himdel mentioned this issue Jul 12, 2017

Fix "space-unary-ops" issue in app/assets/javascripts/controllers/report_data_controller.js ManageIQ/manageiq-ui-classic#1569

Closed

himdel mentioned this issue Aug 21, 2017

[POC/WIP] Angular component with redux settings ManageIQ/manageiq-ui-classic#1944

Closed

Fryguy mentioned this issue Aug 25, 2017

PT#149703867 standardjs/eslint swap ManageIQ/manageiq-ui-service#886

Merged

miq-bot added the stale label Feb 26, 2018

miq-bot mentioned this issue Dec 5, 2018

JS style guidelines (draft) ManageIQ/manageiq-ui-classic#5045

Closed

miq-bot closed this as completed Dec 5, 2018

[WIP] JS style guidelines (draft) #8781

[WIP] JS style guidelines (draft) #8781

Comments

himdel commented May 18, 2016 • edited Loading

formatting

indentation

object and array literals

trailing commas

function calls

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

if/while blocks

expressions

switch statements

naming

functions

angular

globals

don't

ManageIQ global object

general

comparison operators

quoting

es6

linting

himdel commented May 19, 2016

himdel commented May 19, 2016

martinpovolny commented May 19, 2016

himdel commented May 19, 2016

AparnaKarve commented May 19, 2016

romanblanco commented May 19, 2016 • edited Loading

AparnaKarve commented May 19, 2016

himdel commented May 20, 2016 • edited Loading

skateman commented May 20, 2016 • edited Loading

abonas commented May 22, 2016

skateman commented May 22, 2016

abonas commented May 22, 2016

himdel commented May 22, 2016 • edited Loading

abonas commented May 23, 2016

himdel commented May 23, 2016

abonas commented May 23, 2016

himdel commented May 23, 2016 • edited Loading

zeari commented May 23, 2016

himdel commented May 23, 2016

zeari commented May 23, 2016 • edited Loading

himdel commented May 23, 2016

zeari commented May 23, 2016

jkremser commented May 23, 2016

karelhala commented May 25, 2016

himdel commented May 25, 2016

skateman commented May 25, 2016 • edited Loading

romanblanco commented May 25, 2016

skateman commented May 25, 2016

Chained methods

If blocks

AllenBW commented Aug 24, 2017 • edited Loading

skateman commented Aug 24, 2017

himdel commented Aug 24, 2017

chriskacerguis commented Aug 24, 2017

AllenBW commented Aug 24, 2017 • edited Loading

martinpovolny commented Aug 24, 2017

chriskacerguis commented Aug 24, 2017

mtho11 commented Aug 24, 2017

AllenBW commented Aug 24, 2017

himdel commented Aug 24, 2017

mtho11 commented Aug 24, 2017

mtho11 commented Aug 24, 2017

mtho11 commented Aug 24, 2017

AllenBW commented Aug 24, 2017 • edited Loading

mtho11 commented Aug 24, 2017

martinpovolny commented Aug 25, 2017

AllenBW commented Aug 25, 2017

miq-bot commented Feb 26, 2018

JPrause commented Dec 5, 2018

himdel commented Dec 5, 2018

himdel commented Dec 5, 2018

miq-bot commented Dec 5, 2018

himdel commented May 18, 2016 •

edited

Loading

`ManageIQ` global object

romanblanco commented May 19, 2016 •

edited

Loading

himdel commented May 20, 2016 •

edited

Loading

skateman commented May 20, 2016 •

edited

Loading

himdel commented May 22, 2016 •

edited

Loading

himdel commented May 23, 2016 •

edited

Loading

zeari commented May 23, 2016 •

edited

Loading

skateman commented May 25, 2016 •

edited

Loading

AllenBW commented Aug 24, 2017 •

edited

Loading

AllenBW commented Aug 24, 2017 •

edited

Loading

AllenBW commented Aug 24, 2017 •

edited

Loading