Current styleguide conventions with modern JS

[epixa]

The existing styleguide was in great need of a rewrite as it did not
reflect the conventions we're using in the codebase or even the best
practices that we follow. In some cases, the guidance it provided was
outright contrary to our current practices.

For reviewers: For the most part, I recommend just looking directly
at the updated styleguide and reviewing it as if it were a new document
rather than trying to make sense of this diff.

This document really needs to be reorganized with jump links as well,
but for this first pass, please just review the content itself. We can
follow up with an additional PR that reorganizes everything.

Link directly to JS styleguide from this branch: https://github.com/epixa/kibana/blob/styleguide/style_guides/js_style_guide.md

Closes #4949

[kimjoar]

Several of these can be covered by a linter. Documentation tends to get out of date. Maybe either remove those sections that are covered by the linter, or move the documentation into the lint config? (That way it's easier to see what needs to change when we change our practices?)

[kimjoar]

That addUser function that mutates one of its arguments 🙀💀

[epixa]

Great point about the linter, but let's take care of that as part of the followup "reorganization" instead. This way we at least get consensus on intentions from this PR and then the reorganization PR can be focused purely on the logistics.

Good catch on addUser. I originally had all of the "good" examples trying to be as similar in return values and side effects as the "bad" examples, but I ended up going through and changing them all to show better practices even if it meant different results. Missed that one, though.

[epixa]

I added a new section to describe appropriate import scenarios: https://github.com/epixa/kibana/blob/styleguide/style_guides/js_style_guide.md#import-only-top-level-modules

[bevacqua]

My dimes:

• I'd get rid of the pointless newlines rule (it just bloats the style guide for no reason)
• I strongly suggest we encourage template literals for every single string
  • And then remove the "use template literals to avoid escaping single quotes"
• Remove "Opening braces go on the same line", nobody places them anywhere else in JavaScript
• I find return done(err); to be terrible. I would add a rule that states we should use done(err); return; instead because that conveys intent better
• I find it plain stupid that we're allowed to do if (foo) bar(); when we stated one rule earlier that braces must be used in multi-line scenarios. I'd still enforce brackets as in if (foo) { bar(); }
• I'd add a rule to explicitly recommend a => a over (a) => a when there's a single arrow function parameter
• I'd add a rule to explicitly recommend a => ({b:a}) over a => { return {b:a}; } when it comes to returning objects from arrow functions
• I'd rename the regular expression as validPassword without a RE suffix ala Visual Basic
• The "shorten things even further" example is pointless
• On "use of the new operator", clarify that we want to hide them behind an API, but new isn't supposed to completely go away (the implementation would still use it)
• The bolded text is frightened and in the middle of nowhere, please strike it from existence "Do not use setters, they cause more problems for people who try to use your software than they can solve." -> "Do not use setters, they cause more problems than they can solve."

[epixa]

I'd get rid of the pointless newlines rule (it just bloats the style guide for no reason)

Do you mean the linux line-endings rule? What's pointless about that?

I strongly suggest we encourage template literals for every single string

I don't have a strong opinions about it, but that does seem a little overkill. Maybe other people do feel strongly about it.

Remove "Opening braces go on the same line", nobody places them anywhere else in JavaScript

I disagree, though this whole thing will be removed when we follow up with removing items that the linter will catch.

I find return done(err); to be terrible. I would add a rule that states we should use done(err); return; instead because that conveys intent better

Again, I have no strong opinions about this one. I'll make the change unless other people disagree.

I find it plain stupid that we're allowed to do if (foo) bar(); when we stated one rule earlier that braces must be used in multi-line scenarios. I'd still enforce brackets as in if (foo) { bar(); }

It isn't a multi-line scenario, though. That was kind of the point.

// good
if (true) return;

// bad
if (true)
  return;

I'd add a rule to explicitly recommend a => a over (a) => a when there's a single arrow function parameter

I have that in the second Arrow Functions example.

I'd add a rule to explicitly recommend a => ({b:a}) over a => { return {b:a}; } when it comes to returning objects from arrow functions

👍

I'd rename the regular expression as validPassword without a RE suffix ala Visual Basic

👍

The "shorten things even further" example is pointless

It's point in that context is to make up for the fact that the previous example is garbage. The previous example gets the point across, but it does so with a pretty terrible implementation.

So I really should just fix the first example, so we can ditch the second.

On "use of the new operator", clarify that we want to hide them behind an API, but new isn't supposed to completely go away (the implementation would still use it)

Yeah... I'm more than happy for new to completely go away. The only reason I didn't wholeheartedly throw my support behind the PR that @Bargs issued the other day about removing classes is that I don't think all contributors are ready to move on from the paradigm of classes, and if I see a PR that does have a class in it, I want it to be the native class syntax.

But given a newer, less established project, I'd happily disallow classes altogether, and I hope we'll be able to move away from them ourselves some day.

The bolded text is frightened and in the middle of nowhere, please strike it from existence "Do not use setters, they cause more problems for people who try to use your software than they can solve." -> "Do not use setters, they cause more problems than they can solve."

👍

[bevacqua]

Do you mean the linux line-endings rule? What's pointless about that?

It's redundant, I haven't seen \r\n used anywhere for years.

I don't have a strong opinions about it, but that does seem a little overkill. Maybe other people do feel strongly about it.

How is it any more overkill than using const everywhere? Template literals are strictly better than single or double quotes:

• Always able to interpolate expressions, no need to change into template literals because they are already template literals
• No need to escape single quotes
• Multi-line

I really should just fix the first example, so we can ditch the second.

👍

[panda01]

This breaks your no () rule.

[Bargs]

I'd still enforce brackets as in if (foo) { bar(); }

I agree with @bevacqua. Allowing braceless conditionals only saves two characters of typing (1 or 0 if you're using a proper editor) and increases the chances of accidental breakages down the road.

Re: Importing modules - we should also explicitly require the use of relative paths and disallow things like webpack aliases.
On the topic of only importing top level modules: this seems like only part of the puzzle. Most modules in our code base aren't designed with a public API in mind so there's really no "top level" to import. We should probably have a section which offers some guidance on building modules that can be consumed in the way this import section describes.

Classes - This seems like a good start. Based on the feedback from my previous PR, I have some thoughts on offering users more guidance towards options that are better than classes. I can address it in another PR, unless you'd like to have that discussion here.

Chaining indentation - I used to prefer the recommended style, these days I don't have a strong preference. Just wanted to say that if we make this change, our entire codebase is gonna be in violation. Staying consistent might be a lesser evil.

[cjcenizal]

How does everyone feel about deferring to the AirBnB style guide, and using this doc to only document the cases in which we deviate or extend those guidelines?

The AirBnB docs are really well-written, commonly used, and contain customizable linting rules.

I took a look at how closely it matches Court's revisions, and I think the two are very similar (some parts of our style guide have actually been copy/pasted from the AirBnB style guide).

Here are the sections I suggest we keep, since they address topics which AirBnB doesn't:

• Magic numbers/strings
• Import only top-level modules
• Object / Array iterations, transformations and operations
• Use descriptive conditions
• Name regular expressions
• Return early from functions
• Avoid mutability and state
• Use thunks to create closures, where possible

Here are the sections which somewhat deviate or contraindicate from their style guide, so we can decide to keep them or remove them:

• Object / Array creation
• Classes/Constructors and Inheritance
• Getters and Setters

[bevacqua]

I wrote my own take as well (bevacqua/js), but I think we should just iterate over what we have rather than start again from scratch because maintainability. This sort of bikeshedding is helpful up to a point, but then it just gets in the way.

[uboness]

I'd still enforce brackets as in if (foo) { bar(); }

++

Yeah... I'm more than happy for new to completely go away. The only reason I didn't wholeheartedly throw my support behind the PR that @Bargs issued the other day about removing classes is that I don't think all contributors are ready to move on from the paradigm of classes, and if I see a PR that does have a class in it, I want it to be the native class syntax.

But given a newer, less established project, I'd happily disallow classes altogether, and I hope we'll be able to move away from them ourselves some day.

can someone please enlighten me on what's wrong with classes and using new to instantiate them? (beyond the argument that for years javascript didn't have them and everybody tried to mimic them in the most horrible ways... anyway... we're not here to tightly hold on to the past). ES6 is clearly moving towards classes, later specs are also moving towards enhancing classes (e.g. class fields - https://github.com/jeffmo/es-class-fields-and-static-properties).

[Bargs]

@uboness ES6 classes and new suffer from all the problems of classical inheritance and constructors that you run into in languages like Java. Books like Effective Java have long encouraged developers to use static factory functions over constructors, and composition over inheritance. The advice to avoid classes and new in ES6 follows in the same vein. Unlike in Java where everything is a class, we can avoid the issues entirely in JS by ignoring these language features.

In addition to those foundational problems, ES6 classes have another huge implementation issue. Classes make encapsulation impossible. Not only do they not provide a way to create private members, they also prevent you from hiding private instance members inside a closure. To me, this is reason enough to avoid them. What's OOP without encapsulation? This issue will probably be resolved in future versions of the spec. But it won't address the above points.

[epixa]

And a few others:

• Without a factory, you can't partially apply the new operation. You can't defer the operation to be executed later. You cannot compose the operation to create new operations (no pun intended).
• There are no types in Javascript, so defining new classes is just creating new untyped objects. You're just creating a glorified factory function with less control over encapsulation, added complexity, and less flexibility.

[bevacqua]

My 2 grains of sand:

• You can partially apply in ES6 using spread: new Foo(...['a', 'b'], 'c', 'd')
• Another pain point is that ES6 classes don't have their own scope where you can declare functions private to the class. This is coming, alas

[epixa]

@bevacqua Partial application in the functional programming sense: you can't create a new operation that already has some of the arguments applied. Though, as you pointed out, you can invoke the constructor with variable arguments now.

[bevacqua]

Well, pre-ES5 if you really really wanted you could do new + apply, although I'm only raising this just to harrass you at this point:

new (Date.bind.apply(Date, [null,2015,31,8]))

See:

• http://stackoverflow.com/a/8843181/389745
• https://ponyfoo.com/articles/es6-spread-and-butter-in-depth

When it comes to partial application you could consider new as being "lost" as part of the partial application:

function curried (...rest) {
  return new Foo('a', 'b', ...rest);
}

[uboness]

Books like Effective Java have long encouraged developers to use static factory functions over constructors

A factory has its place where/when it's needed. Having a general rule of "replace all ctors with factory methods" is plain nonsense (sorry). A factory is good when you potentially have multiple implementation of a class and you want to hide those away. Another reason to have factory methods is for convince and readability... at times a class can be instantiated in multiple ways with multiple args, and factory methods better document the nature of those.

composition over inheritance.

+1 on that... has nothing to do with classes vs. functions/factories

Unlike in Java where everything is a class, we can avoid the issues entirely in JS by ignoring these language features.

not everything in java needs to be exposed as a class - you can still hide things behind static methods. That said, having classes provide structure to your constructs (with or without having factory methods around)

In addition to those foundational problems, ES6 classes have another huge implementation issue. Classes make encapsulation impossible. Not only do they not provide a way to create private members, they also prevent you from hiding private instance members inside a closure. To me, this is reason enough to avoid them. What's OOP without encapsulation? This issue will probably be resolved in future versions of the spec. But it won't address the above points.

I can live with this argument (as opposed to the previous ones). That said, one could argue to prefer clearer structure over language level enforcement of visibility (not saying I do... just saying it's an option). Also, you could still use classes and work with module level variables (they are bound to the module itself, don't they?)

Without a factory, you can't partially apply the new operation. You can't defer the operation to be executed later. You cannot compose the operation to create new operations (no pun intended).

as mentioned above, factories serve their purpose when needed... no (and never did) arguing factories should never be used

There are no types in Javascript, so defining new classes is just creating new untyped objects. You're just creating a glorified factory function with less control over encapsulation, added complexity, and less flexibility.

Indeed... today these are glorified factory function and (as mentioned above) visibility is broken. The reason I raise it though is that it seems to me that JS is moving more towards adding those:

• https://github.com/wycats/javascript-private-state
• https://github.com/sirisian/ecmascript-types

Look, I've done enough Java, OOP and built a few large scale extensible systems to know the pros & cons of classes, constructors and inheritance. I'm not here to start a war, not on JS as a language, or with JS developers or on Java & OOP. Javascript is historically notorious (whether ppl admit to that or not) for its pitfalls as a language. Traditionally, entire large JS code bases, very quickly, reached an unmaintainable state (call it "function calls and variables a la bolognese"). In recent years you clearly see a shift though... with nodejs came great responsibility and what you actually see today is JS following a very similar path to that that Java did 15 years ago. Where it tries to figure out itself and its platform/echo-system. It's not for nothing you have 50+ JS logging frameworks out there, 50+ http frameworks, 50+ IoC frameworks and every month someone declares they found a better way to develop web apps (react or not to react... that's the question.. no... angular or not to angular... wait, angular 1.x or 2.x?... ayayay... ember anyone?). ECMAScript is being enhanced all the time and following the direction it's all going now, I can't help but feeling "I've seen this before". Also, asking myself, why are modern frameworks like angularjs & react move towards the notion of classes?

So with all this rumble jumble and whathaveyou... for me, the questions are very simple. Are we sure we want to ignore classes and keep using old JS style, just because it's not perfect yet? In a year time, will we regret it? Will we have this urge to rewrite the whole thing to keep it somewhat modern? Or... alternatively, we can accept the changes, accept the pros as well as their cons that come with them, but with that, make it somewhat easier for us to enhance over time, along with JS & Nodejs.

[epixa]

Not to nitpick, but this PR doesn't propose disallowing the use of classes, and it makes it clear that if you are using a class, it should be a native class.

[Bargs]

As @epixa said this is really beyond the scope of this PR since it doesn't ban classes, but I would like to clarify a few points.

Having a general rule of "replace all ctors with factory methods" is plain nonsense (sorry).

In Javascript today, I disagree. There's no use case where an ES6 class is a superior option to a factory function.

+1 on that... has nothing to do with classes vs. functions/factories

It does, because classes encourage classical inheritance which couples sub-classes to their super classes.

Also, you could still use classes and work with module level variables (they are bound to the module itself, don't they?)

Yes, but there's no way to implement private instance members without some really convoluted code involving symbols or weakmaps.

Are we sure we want to ignore classes and keep using old JS style, just because it's not perfect yet? In a year time, will we regret it? Will we have this urge to rewrite the whole thing to keep it somewhat modern?

I agree 100%, we should try to minimize future regret and that's why we should avoid classes for now. The beauty of factory functions is that they give us flexibility in the future. If classes turn into something awesome, we just update the factory functions to return class instances.

If we use classes extensively today and classes evolve into something terrible (or a better option appears), we're screwed, because classes cannot be used without the new keyword and the new keyword couples callers to the details of object instantiation. Converting classes into factory functions is much more painful than modifying factory functions to return class instances.

[bevacqua]

[Bargs]

That shed would look much nicer in red, don't you think?

[bevacqua]

Red would be too prominent. Bikes wouldn't stand out.

[uboness]

In Javascript today, I disagree. There's no use case where an ES6 class is a superior option to a factory function.

it's not about superiority (at least today), it's about coding style and consistency (see below)

As @epixa said this is really beyond the scope of this PR since it doesn't ban classes, but I would like to clarify a few points.

wrong. the code need to be consistent. (as in .). If factory method is what we're going for, then factory methods all the way. If we don't want new keyword, then it's all the way. Style is not all about superiority of one feature over another, it's about building a coherent code base, where no matter where you land, you always feel at home. Where if you need to implement something, you don't ask yourself too many questions like (should it be a class, a factory method, an object notation or a proxy to a bridge over a shed). Coherency is something kibana code base lack today... and we will fix this.

It does, because classes encourage classical inheritance which couples sub-classes to their super classes.

1. there's nothing wrong with inheritance (of course, it, like everything in this world and in programming specifically can be abused).
2. classes don't encourage inheritance more than functions encourage pure functional programming.. so when was the last time you've used cdr and cadr in JS?

Yes, but there's no way to implement private instance members without some really convoluted code involving symbols or weak maps.

I definitely accept that as an argument... I'm all for less convoluted code

I agree 100%, we should try to minimize future regret and that's why we should avoid classes for now. The beauty of factory functions is that they give us flexibility in the future. If classes turn into something awesome, we just update the factory functions to return class instances.

If we use classes extensively today and classes evolve into something terrible (or a better option appears), we're screwed, because classes cannot be used without the new keyword and the new keyword couples callers to the details of object instantiation. Converting classes into factory functions is much more painful than modifying factory functions to return class instances.

I can live with this statement. I don't necessarily agree one thing is easier than the other though. converting from one paradigm to another will come with a cost, potentially high cost either way. Again, I'm raising this simply as I see a pattern of where JS is heading... nothing more.

This discussion could not be more relevant to this issue. This IS style... and here we should sort it out and explicitly explain (in words, in the guideline doc) why we choose one thing over another.

And no... the shed would not look good in red, unless you plan to park a firetruck under it.

[bevacqua]

@epixa We could just use eslint which supports a --fix flag that
autofixes most stylistic changes

On Fri, Sep 16, 2016 at 3:49 PM, Court Ewing notifications@github.com
wrote:

@cjcenizal https://github.com/cjcenizal I think the purpose of this PR
is getting skewed, and any talk of adding new linting rules or adopting a
new styleguide is just reinforcing that. This PR is updating our very
outdated styleguide to reflect the code we're actually writing every day.
The code we produce right now in nearly every PR and even our own current
linting rules do not reflect reality, so this PR is a bug fix rather than
an enhancement.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7435 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA5BlRsyx9PahCeb0lYOpHT3NZAQTPfwks5qquSngaJpZM4Izzd9
.

https://ponyfoo.com/ · github https://github.com/bevacqua · twitter
https://twitter.com/nzgb · articles https://ponyfoo.com/ · newsletter
https://ponyfoo.com/weekly · books https://ponyfoo.com/books · speaking
https://ponyfoo.com/speaking · career
http://careers.stackoverflow.com/bevacqua · about
https://ponyfoo.com/about

[epixa]

@bevacqua eslint only fixes a small portion of possible linting mistakes due to the way it executes. It's a start, but a full migration would require more automation than that.

Inline comments addressed, but airbnb styleguide is a separate effort than this

[epixa]

Alright, I've updated this to address the feedback from the inline comments, which were all about adding more clarity/rationales behind the guidelines. Even though we will be switching to the airbnb styleguide, I'm still going to push this change in the meantime so our styleguide isn't outright incorrect like it is right now.

I'll push this on Monday barring any additional feedback.

[cjcenizal]

LGTM

[spalger]

This looks like a great update, LGTM. Does this mean we can restart work on #8101?