JavaScript Standard Style

[chillu]

Introduce https://github.com/feross/standard. While the CMS JS will be rewritten to ES6 over time, I think it's still important to get all of it over into a modern development flow - and that starts with linting and coding standards. I'm hoping to avoid a discussion about semistandard vs. standard ;)

Most of the syntax changes were performed automatically through standard --format. I've fixed up mainly var declarations. The main risk here are manual conversions to type-safe comparators (=== instead of ==), but I've reviewed each use case before fixing. Overall, I think we have good enough Behat test coverage to catch out any serious regressions, plus we're still a fair bit away from a stable release on 4.0, giving us a chance to clean things up.

@flashbackzoo Yes, this is going to complicate merges from https://github.com/open-sausages/silverstripe-framework/tree/integrate-react, happy to help with the rebase on that. Better now than never.

The standards aren't enforced through CI here yet, but keen to do that soon (before the new asset-admin work gets merged).

Merge together with silverstripe/silverstripe-cms#1405.

Relies on #5106 merged first (shrinkwrap changes).

/cc @unclecheese @flashbackzoo

[flashbackzoo]

I find one line if statements a lot less legible. Scripts are minified so legibility is more important than LOC in source files. I like using semicolons for legibility too, they infer intention, you know the developer meant to end the statement there and hasn't just forgotten to chain a extra method call or something.

Having a coding standing is important because, it's currently all over the show, and I like standard style except for the points I mentioned above.

Yeah - I guess. This is better than what we have.

👍

[chillu]

I find one line if statements a lot less legible

@flashbackzoo So you're referring to the fact that one line if is allowed by standard?

In terms of semistandard and semicolon support, just going by github stars/watchers/forks and npm insatlls, standard is at least 10x as popular. So I think we're backing the option with more traction here, meaning developers will become increasingly used to reading and writing code in this style. It's definitely more familiar for CoffeeScript/Ruby/Python devs than PHP devs, but I think we can handle it - particularly with the great IDE hinting support with standard provides.

/cc @assertchris

[flashbackzoo]

So you're referring to the fact that one line if is allowed by standard?

Yes.

I'm not too stressed either way about semicolons, definitely not worth getting into a lengthy debate over. My personal preference is to use them, but the goal is to make our codebase accessible for the majority of people, so I'm happy to go with whatever achieves that 😄

[assertchris]

I think anything as contentious as a global style change should be put to a vote by the core contributors, just like what happened for PSR-2.

[chillu]

@silverstripe/core-team Can you please add your +1 or -1 (with reasons and alternative suggestions) here in the next 48h? I'd hope this is much less contentious than adopting PSR-2, given how much more (PHP) code it affects.

[assertchris]

I'd hope this is much less contentious

Ask about semicolons in the mailing list...

[dhensby]

No semi colons?! The F? @chillu, can you cite other projects using this to convince me. I'm on the fence. Are there other style guides?

[chillu]

Hah OK, I was asking for this. Here's the thing: I care about agreeing on a standard. I don't really care about semicolons. And there's basically standard (no semicolons) or semistandard (semicolons). All other rules are the same in there. So as long as we agree that either one of these works for us, I'm fine. What I'm not fine with is spending a lot of energy on arguing over other stylistic details. Adding in semicolons to this PR is as simple as running semistandard-format -w. So I'm happy to format the code both with and without semicolons, and cast my own vote for leaving them out.

[chillu]

Also, thanks everybody for responding and caring. It's been a long day with a few yak-shaving moments already :D

[dhensby]

I think agreeing a standard over the details is a priority too; I was happy with PSR2 even though tabs > spaces :P

My understanding is leaving out semi-colons can have unexpected results and lead to people making mistakes that aren't obvious. You'd also be at the mercy of the ASI logic in each browser which shouldn't, but could, vary from browser to browser.

I'm a bit confused because the diff you have has some semicolons added and some removed so it doesn't appear consistent from first viewing... Though maybe I need to RTFM to learn what "no semicolons" means.

[kinglozzer]

@silverstripe/core-team Can you please add your +1 or -1 (with reasons and alternative suggestions) here in the next 48h? I'd hope this is much less contentious than adopting PSR-2, given how much more (PHP) code it affects.

+1 for adopting a standard. Either of those looks good.

My personal preference is to include semicolons, mainly because it’s not immediately obvious to me where they can/can’t and should/shouldn’t be omitted: I had to read the blog posts linked from the standard readme, and I’ve already forgotten the rules as I type this :D. If it’s something that can be fixed automatically (which it appears it is) then I’m far less concerned.

[jonom]

+1 for standard. The standard standard I mean, not the semistandard standard.

I'm not a fan of having to use spaces instead of tabs in PHP now but I accept that a bunch of people debated this for a long time and spaces won so there must be a good reason for it. If standard is the javascript equivalent of PSR-2 I say let's run with it, smart people have already debated everything and decided this is the best standard for javascript. Might be a bit uncomfortable at first but I don't think we should try to treat javascript like it's PHP and require semi-colons for the sake of it - they're different languages so deserve different rules. I didn't realise semi-colons aren't commonly necessary in javascript, so standard has expanded my knowledge of the language already!

I'm a bit confused because the diff you have has some semicolons added and some removed so it doesn't appear consistent from first viewing... Though maybe I need to RTFM to learn what "no semicolons" means.

Agree, still seem to be a lot of semi-colons in this PR, like this example from admin/javascript/dist/MemberDatetimeOptionsetField.js

onclick: function onclick(e) {
  jQuery(this).closest('.description').find('.toggle-content').toggle();
  return false;
}

[jonom]

My understanding is leaving out semi-colons can have unexpected results and lead to people making mistakes that aren't obvious. You'd also be at the mercy of the ASI logic in each browser which shouldn't, but could, vary from browser to browser.

@dhensby this is apparently a myth, this short post seems to sum it up well and convinces me that including semi-colons can lead to more confusion than leaving them out. http://blog.izs.me/post/2353458699/an-open-letter-to-javascript-leaders-regarding

Here's an example where relying on semi-colons could get you in to trouble

return
    7;

Means this in PHP

return 7;

Means this in JS

return;
7;

[chillu]

On the semicolons in the diff of this PR, it's the minifier putting them back in - to save a few bytes by contracting lines. You'll only find these in the */dist/* folders, not in */src/*.

Regarding memorising the rules when writing JavaScript without semicolons, there's really only one practically relevant rule: "Never start a line with ( or [ or ```" (details). And that's automatically checked for you through standard in most IDEs with the appropriate plugin. Plus I'm planning to run `standard` via Travis, so this will get noticed before a core merge.

[sminnee]

My vote would be for semistandard over standard, and for standard over anything else. I'm happy to go with the consensus if it favours standard. My main rationale for favouring semistandard is that it feels like a more coherent fit with a predominantly PHP project. That said, if there is a strong preference for standard over semistandard in the JS community I'd probably follow the JS community's consensus.

I opposed to anything other than one of those 2 options, and strongly opposed to tweaking the standard.

[sminnee]

...okay, semistandard is 11k downloads/month and standard is 150k downloads/month. The JS community hates semicolons.

[sminnee]

@chillu I know it it's like hair-growth formula for your yak, but I think we should cross-post this to silverstripe-dev, so people are made aware of it.

[stevie-mayhew]

😆 @sminnee. Would love to see the yak.

Any decision here is fine if its automatically linted. We use https://github.com/airbnb/javascript internally and it works well, because we enforce automatic linting.

[tractorcow]

+1 on https://github.com/feross/standard. I used the same reasoning as @sminnee , lol, looking at download numbers. :)

[chillu]

Tally so far looks to be slightly in favour of standard. Or rather, there's a lot of people that are happy with either of standard or semistandard, which is great to hear. Please let me know if I've misinterpreted anybody's feedback. I don't want to leave this PR open for too long because it'll get harder to rebase over time.

semistandard: (none)
standard: Ingo, Damian, Jono, Sam (?)
semistandard or standard: Stevie, Loz, David

If you are still concerned about remembering the few rules about omitting semicolons, please remember that these checks will likely be enforced at compile time through gulp. So the chances of causing browser bugs or functional regressions are fairly low. We can also be proactive about recommending to any CMS JS authors to install an IDE plugin, which will catch any syntax errors before even saving the file. Combined with CI, it's pretty much on the same level as potentially committing broken PHP - it's just not very common.

@hafriedlander Any comments on this? You've probably authored more JS in your life than most other core members :)

[anselmdk]

I love having a standard. I'd been confused about leaving those semicolons out if I hadn't been writing a lot of React/ES6 lately, where I tend to often leave those out (though not following a standard). If the JS community thinks we should leave them out, I'd vote for that.

[NightJar]

Semicolons can get off this bus. I'm OK with that; It's Javascript.

Although I have a distaste for spaces preceding(bracketed, arguement, lists) {/* declaration or otherwise */}, it is a different category from visceral repulsion, so I'll let it slide. standard seems like a sensible option in standard selection, thus gets my (somewhat insignificant) vote.

[markguinn]

I agree with NightJar: I don't love the space before function arguments but I think a consistent and community supported standard is more important. I bet my brain will get used to that space pretty quickly.
+1

[oetiker]

I think it is a really bad idea to loose the semicolons ... http://stackoverflow.com/questions/444080/do-you-recommend-using-semicolons-after-every-statement-in-javascript ... javascript is neither ruby nor python ...

google is also pretty clear on this: https://google.github.io/styleguide/javascriptguide.xml

[stevie-mayhew]

I would consider looking at the styles which are used in contributing modules in the ecosystem we are becoming a part of, i.e. the React ecosystem.

It would be good to audit the current dependencies for the asset work going on and see if there is a prevalent style in use. I imagine that it isn't standard 😉

If there is a general style which is used by these modules and systems then I think it makes sense to go with whatever coding standards most closely match these.

If you want me to do this, let me know and I'll go through and put together some documentation on current styles vs proposed with standard/semi-standard/others.

It makes sense for us to lean on the code we are using for style guidelines, rather than going down a path which isn't consistent with it. Basically the PSR-2 or own-style-guidelines argument but for JS where there isn't a PSR-2 to go to which is in use basically everywhere.

[patricknelson]

-1 on standard, +1 on semistandard on to basis of semicolons.

I understand the arguments on both sides. I'm primarily against it on aesthetic grounds (just easier for me to read, not hard to type). I don't find it more fatiguing to decide on where to write semicolons, as it's simple to learn and understand (either way). On a more practical note, just with all the current code, refactoring the codebase will likely make blaming much more difficult as time passes. So yes, I know "we should keep doing it this way because this is how we've always done it!" is usually a crappy argument, except in this case it may make it more difficult to track down the source of changes over time.

Other than that, I don't really have a strong argument against standard except for aesthetics and blaming/history. Also, does anyone have more empirical evidence on the basis of this being "the way things are going"? And yes "smart people" do it this way, as many smart people do it other ways. You'll be amazed at the stuff that smart people disagree on, too.

[chillu]

@stevie-mayhew That's a good point, thanks for raising it! I would assume there's a range of standards in our dependencies, and it would be great to see if there's any trends.

React is using the AirBnB style, which comes with its own .eslintrc rules. Due to eslint, we have similar IDE and automation support to standard and semistandard. It has 290k downloads a month, which is twice as much as standard.

So on paper, we could go with AirBnB. It also requires semicolons ;) And it has the advantage that we could also adopt its React Style Guide. As I've said earlier, I'm mainly interested in getting a standard in there without spending too many brain cycles on it :) Stevie, if that is your preferred option, can you investigate how much manual fixing we'd have to do on top of eslint --fix automation? If it comes down to "amount of work left", standard or semistandard win at the moment ;)

@patricknelson Yeah git blame and git merge won't be very nice after this change, but now is actually the best time to make this change for JS: We're planning to write and rewrite a whole lot of it in the coming months.

[sminnee]

OK, so the AirBnB standard is more popular than standard, it's adopted by React, and it's a necessary precondition of following the React Style Guide? That changes things for me, and makes me more in favour of using the AirBnB standard.

What are the negatives?

[stevie-mayhew]

@chillu its probably a bit of work. I'll have a look into it tomorrow and see if we can automagic it.

[dhensby]

Airbnb standard gets a +1 from me

[patricknelson]

The AirBnB looks better actually, but my only gripe now is the indentation but again I guess that's aesthetic. I do like the standardization of the use of 'use strict';. Overall +1 for me on AirBnB with a minor caveat.

[nedmas]

I prefer the AirBnB standard over both standard and semistandard. And the AirBnB React style guide looks great. +1 from me.

[hafriedlander]

+1 for AirBnB from me too (and -1 for standard, blech). It is pretty exclusively ES6 though - I guess we're OK with just all JS needing a transcompile for IE10.

BTW, I suspect there's only a weak correlation between download numbers and actual adoption. For instance, standard is used by bootstrap, so many bootstrap users will download it, but that doesn't necessarily equate to strong exposure by devs to that standard.

[scott1702]

+1 AirBnB

[jonom]

AirBnB looks like the clear winner here but if you're still counting votes Ingo I'll be a +1 for 'any standard'

[stevie-mayhew]

Looking into using AirBNB @chillu it looks like you can automate a lot of it through an IDEA editor and automatically "lint" most of the errors. Do you want me to open a new PR or are you happy to update this one if AirBNB is the standard we choose to adopt?

[patricknelson]

Is anyone interested in adopting a modification of the AirBnB standard? Only modification being indentation using 4 spaces instead of only 2?

[dhensby]

I don't think so. It's a trivial point and not worth deviating for.

[sminnee]

Is anyone interested in adopting a modification of the AirBnB standard? Only modification being indentation using 4 spaces instead of only 2?

I am quite strongly opposed to this. It creates unnecessary fragmentation.

[chillu]

Alrighty, AirBnB it is then! Thanks for participating everyone :)

I've made a start on converting the new SS4 asset-admin module to it already. Feels a bit like a pedantic mother sometimes, but in the end mother's know what's best, right? open-sausages/silverstripe-asset-admin@bf77741. Great Atom integration BTW, http://eslint.org/docs/user-guide/integrations

We're having an internal Hackday tomorrow, I'm hoping somebody will pick up the conversion for framework as well.

I'm going to close this pull request for now, since by now its hopefully outdated - and while standard mostly overlaps with AirBnB conventions (e.g. indentation), it's probably less work to reimplement the syntax changes than to deal with the rebase.

[chillu]

AirBnB standard will be introduced to framework via #5278