Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib

+1 ... using a standard followed by React is the key argument.

While I actually prefer the cuddled else style in general, having this be different from our PHP standard doesn't make total sense to me. Same can be said for the object spacing (though we don't really do this in PHP with objects, there is a correlation with arrays).

#7, I think we overestimate the number of people who use proper IDEs, let alone the number of people who have coding standards configured properly for them on a per-project basis. And auto-formatting can have the unintended consequence of adding out-of-scope changes to files.

While I don't do a lot of JS for Drupal and I don't call my self JS engineer, Meteor is part of my day job. I think the airbnb standards are good, and linking out to them is fine (but, I think we need to be pedantic and make sure we link out the the same tag that is in the patch we commit, v12 right now). I like the fact that they show bad and good examples, along with justification in most cases (eg, trailing comma on arrays leads to cleaner diffs). There is greater-JS community adoption.

My primary concern has to do with some of the practicalities of adhering to them. The comment I made about the cuddled else was from the POV more from code review rather than writing code. If a patch has PHP and JS changes in it, there are two else styles in the same patch along with some other whitespace and potentially nuanced differences. Can someone adequately review a patch without local linting? Or, since we have a mostly lintable standard, does adopting this meant that part of the build process should include an `eslint --fix` (forgive me if this was discussed elsewhere)?

For the things that can't be linted and patches that contain both PHP and JS, are we going to have enough people who know these new standards (IOW, are we going to create logjams with patches waiting for review)?

Those questions may have more to do with ES6 than airbnb. And, don't get me wrong, I *want* both ES6 and a well-adopted JS standard to happen. I think it is important, though, to talk of all of the practicalities through. We are in a somewhat unique position where we almost get a fresh start with JS. We have PHP coding standards that we try very, very hard to follow, but phpcs isn't a practical tool for most patches because of variance from the spec that has crept in over the years. Let's make sure we do this right.

Consider this a +1 for what we will eventually land on when we have hammered out the minutiae.

The Drupal PHP code standard always different to JS or CSS. For example: #652650: Change 'elseif' standard to 'else if'

I'd suggested to change some PHP style in Drupal also. e.g. `else if`...
http://www.php-fig.org/psr/psr-2/

Personally, I never read the PHP & JS & CSS code style pages. Just get used to.. Luckily, my patches seldom rejected by code style. For our new developers, I will only tell them to understand the basic points. And not to read the Bible page in Airbnb until you run into trouble.
https://facebook.github.io/react/contributing/how-to-contribute.html#cod...

Keep consistency is good but it should not first priority in our patches

#7, I think we overestimate the number of people who use proper IDEs, let alone the number of people who have coding standards configured properly for them on a per-project basis. And auto-formatting can have the unintended consequence of adding out-of-scope changes to files.

The way how we solved / still solve it for php is the following:

• Start with the set of rules core supports as of now
• Fix a particular rule + expand the set of rules
• Iterate until all rules are declared

See #2571965: [meta] Fix PHP coding standards in core for more details.

IMHO the same kind of approach should be taken for JS as well.

This for example results in the huge improvement, that code reviews never have to think about CS anymore, as things are automated all the time.

One general question, is there a tool phpcbf, which fixes CS issues automatically, for JS, I guess jscodeshift is that tool?

Just a note:

- In these missions, we can run both JSCS & ESLint. JSCS has a longer history in autofix. Hint: Run ESLint twice to get a more optimized code.

Data by count:
https://gist.github.com/KayLeung/ead8ad996a3c99efe11618f09d317a70

`no-param-reassign` http://eslint.org/docs/rules/no-param-reassign
This is real headache.

`func-namec` http://eslint.org/docs/rules/func-names
We able to fix it with jscodeshift I think

Most of ESLint (Airbnb) errors, we intended to do it so. Just no explicit rules in current checks.

This is a blocker of other ES6 issues now.

Can we turn some rules into `warnings` if the automated script can't fix them? We also have a final round of manual code refactoring.

@nod_ this issue is currently on the list of coding standards changes that will be announced for final discussion. This post will be made in the Core group. We don't currently have an ETA for this announcement, but it will probably be in the coming weeks.

I'm +1 on adopting the airbnb standards. Much easier to use someone else's rather than our own and the main JS contributors are in favour. So let's get this done.

+++ b/package.json
@@ -6,12 +6,18 @@
+    "eslint-config-airbnb": "^12.0.0",
+    "eslint-plugin-import": "^1.16.0",
+    "eslint-plugin-jsx-a11y": "^2.2.2",
+    "eslint-plugin-react": "^6.3.0"

Should this fix on a version so we don't get outdated when they advance or change something and then we update at our own leisure?

It's outdated now. eslint-config-airbnb v13.0.0 was released yesterday. A package.json file with the latest versions would have:

+++ b/package.json
@@ -6,12 +6,18 @@
+    "eslint": "^3.9.1",
+    "eslint-config-airbnb": "^13.0.0",
+    "eslint-plugin-import": "^2.2.0",
+    "eslint-plugin-jsx-a11y": "^2.2.3",
+    "eslint-plugin-react": "^6.6.0"

pardon, why it takes so much time to make an announcement? I think the code standard group should ask for help if you're having heavy workloads. (and more public information to let us understand your pain)

Looking back other announcements. It will spend 6 months (average, or a year with no results) to make a simple code standard. We lose the prime time to make great changes.

@droplet I have no idea. I'm +1. Both maintainers agree and since we're using eslint to enforce the standards this will no burden to core committers or the community.

This got announced for final discussion by the TWG - https://groups.drupal.org/node/515644.

@alexpott - Appreciate your pointing that out - forgot to update the tag. Thanks.

This recommendation is approved by the coding standards committee on December 20, 2016.

Moving to core queue per step 7 of https://www.drupal.org/project/coding_standards. Note that this does not yet have the "Core Committer Approved" tag on it, and the core committers might postpone that tag until Core is patched to comply. See also #2831543: Improve and clarify the core committer signoff process for changing coding standards.

Moving to the actual core queue :)

@nod_ the patch needs a reroll.

The core committers have discussed this on recent calls and no objections were raised. Therefore adding the "Core Committer Approved" tag. Given this will involve updates to the shipped .eslintrc setting back to needs work.

Is there any discussion about using exceptions to the airbnb standard?

The patch adopts the standards wholesale, which means no use of "++". I find that incredible useful and readable opposed to the airbnb, and so do several others in What rules do the community tend to override?. I am also reviewing adopting these standards for a team, and some of the rules such as no-plus-plus seem overly restrictive.

I do not see much value in being overly restrictive with the following rules:

- consistent-return: It's useful and easy to read for short functions.
- no-plusplus: It's useful and easy to type and read.
- no-underscore-dangle: It does not matter. Why have a rule if it doesn't matter? Let people have their social contracts.
- define-where-used: I like organizing my vars at the top. I know I could be creating some variables outside of block scope, but meh, it's not that bad of a practice. Do we really care that much?

Updating standards is useful.

Apparently missing from this conversation is consideration of any non-technical implications of adopting a particular standard.

Do we have criteria to decide which external standards we adopt and, by adopting, promote? If not, we should.

Does Airbnb represent the strongest fit with the Drupal mission and principles? Do private, proprietary, and corporate branded standards best represent the vision the Drupal community chooses to promote for the internet?

The three projects listed in the issue summary as using the Airbnb standards are all corporate products. As a model for Drupal, it would seem more relevant to see what other community open source projects are doing.

With no context on other potential options for coding standards, this issue at present doesn't provide enough information to make an informed choice.

Not a full answer to #33, but for now...

Do private, proprietary, and corporate branded standards best represent the vision the Drupal community chooses to promote for the internet?

https://github.com/airbnb/javascript is MIT licensed, so not "proprietary".

In terms of "private", the Coding Standards Committee did discuss this, and decided that there's 2 ways in which we can ensure that AirBnb doesn't "control" Drupal's coding standards:

1. What we're approving here is a specific tag of the MIT-licensed style guide: specifically v13.0.0. Any future changes that AirBnb makes to that guide will require a new coding standards issue for Drupal to update to that version. Thus, the Drupal project retains control of Drupal's coding standards.
2. We can override or extend the AirBnb style guide as appropriate for Drupal via our normal coding standards process. For example, new coding standards issues can be filed for the suggestions in #32. We'll need to figure out how to best document what Drupal chooses to override or extend from the base style guide.

Retitling this issue to clarify that.

In terms of "corporate branded" and "promote", can you clarify your concern? One concrete example I can think of is that we might not want to list Drupal in https://github.com/airbnb/javascript#in-the-wild. Or at any rate, we would need an issue to decide on whether to, and figure out which Drupal governance body to send that issue to.

With no context on other potential options for coding standards, this issue at present doesn't provide enough information to make an informed choice.

No one on this issue suggested another option, even during the 2 weeks between #27 and #28. Is there any other decent option worth considering?

Refining the issue title further

In terms of "corporate branded" and "promote", can you clarify your concern?

Sure.

One of our Drupal principles (disclosure, I wrote the first version of them) is:

Standards-based: Support established and emergent standards

A standard is more than a set of technical rules. It's also a commitment to and statement about a set of values. As much as a practical concern to avoid duplication, the moves to "get off the island" or embrace "proudly invented elsewhere" have at their base an ethical commitment to collaboration and cooperation on shared solutions and standards.

When we adopt a standard produced by an entity, we are endorsing and promoting that entity along with their standard. That's a big part of the point. We are saying: when we do X, we look to Y as an authoritative source, and we encourage others to do likewise. Aren't we?

It's not irrelevant to note that, broadly, there's a tension between the cooperative, open internet of free software, on the one hand, and the internet as a set of increasingly powerful, private, proprietary platforms. Choices we make may increasingly tend to commit us to one or the other side of this tension.

That's the light in which I think we need to consider a statement like the one that went out today on Drupal Planet's Drupal governance announcement feed: "Adopt airbnb javascript coding standards."

Is there any other decent option worth considering?

There is of course no shortage of Javascript coding standards or style guides. Without suggesting they're necessarily appropriate, a few are:

• standardJS
• Mozilla coding style
• jQuery style guide
• Angular style guide

In the issue summary the ember.js standards are described as "extremely similar" to those of Airbnb. So, presumably, that would be another option to consider.

It's biased. I'm the one who first suggested to adopt Airbnb JS Guide into Drupal in another issue thread.

Here's why I chose it:

- Popularity
We don't want to create Drupalism. It's removing the contribution barrier to JS in Drupal.

Airbnb is the most starred in Github.

It's absolutely wide used:
https://github.com/airbnb/javascript#in-the-wild

- Tooling
JS is fatigue. At 2016 and coming 2017, we needed tooling to help us finish the job well. We hope everyone to use & have the tooling on their computer already. We don't want to teach them and refer them to a Drupal Guide to do Drupalism setup only. They need not understand things with limited resources.

More importance, all of these tools should be well covered the daily tools we've used. For example, popular editors: VSCode, PHPStorm, Sublime..etc

- Maintenance
Our choice should be open to contribution. And have well commit records. And the whole community providing tools for it.

We don't want to maintain & FAILED to maintain tooling for our developers. It's why I suggested to pick other guides rather than make our own thing.

- Quality
Reasonable Rules rather than personal preferences. Strictly than Loose.

Also, I hope we have a better guide for our developers rather than ESLint rule only.

Airbnb is the only one doing that. Their guide clearly showed BAD, GOOD, BEST examples.

- Others
Less or More, there must some rules you don't like. We should think in global than personal

I agree with nod_. We do need to adopt the standards because it's critical for es6 development with new language features such as rest/spread, arrow functions, etc..., but I don't think we need to adopt all of them so strictly/stringently.

I could see myself coming around to define-where-used, but it's personally very jarring to see in code. I still feel the other rules that I mentioned are too strict for the value that they provide.

https://github.com/airbnb/javascript is MIT licensed, so not "proprietary".

I am not a layer, but I want to point out that the MIT license covers only documentation and code example in that repository. The "coding standard" itself remains an intellectual property of airbnb and can even be patented (if it makes any sense to do that).

For example, take a look at the SQL. Everyone uses it, but it is not an open standard, it belongs to ANSI, and you can buy it for a couple of hundred bucks.

@nod_

I rerolled this. Additionally I'm not sure we need to include eslint-plugin-react since I don't believe we'll be supporting JSX anytime soon.

+++ b/core/package.json
@@ -6,13 +6,17 @@
+    "eslint-config-airbnb": "^12.0.0",

Per #34, don't we need to pin this to exactly 13.0.0?

@mpdonadio

The ^ allows a compatible version to be installed. Have a look at the NPM docs for more information. If we really want to be safe, we should probably implement a shrinkwrap file.

#43, I should have taken more time with my comment. In #34, there was:

What we're approving here is a specific tag of the MIT-licensed style guide: specifically v13.0.0. Any future changes that AirBnb makes to that guide will require a new coding standards issue for Drupal to update to that version. Thus, the Drupal project retains control of Drupal's coding standards.

So, the patch has has to be either "^13.0.0" or "13.0.0" (tbh, I am not sure what previous versions have changed in minor or patch versions to know whether we can carat or not).

We have a composer.lock, so we should really be providing a npm shrinkwrap, too, but I am not sure if that is in scope for this issue.

Here's another way of putting the contrast I made above between the open internet of free software and proprietary platforms like Airbnb:

many argue that ... it's only a matter of time before walled gardens like Facebook and Google — and the native applications which serve as their gatekeepers — overwhelm the web as we know it today: a public, inclusive, and decentralized common good. ... [However,] I'm hopeful Drupal can exemplify how the open web will ultimately succeed over native applications and walled gardens.

That's from a blog post earlier this year by Dries Buytaert.

Okay, I acknowledge a bunch of good points made, including:

• need for support for EMCAScript 6 structures
• priority of getting a solution in place to remove blocker to other issues
• technical merits of the Airbnb style guide
• importance of support for solution from current key Drupal JS contributors.

So are there options that might meet these needs while not promoting corporate "walled gardens"--and while retaining the ability to adapt a standard to our needs?

Two potential strategies.

1. Select one of the other existing JS style guides that closely meets selection criteria and engage with the project to see if we can fill in the ES6 gaps. This approach has plenty of precedents. For example, when the licensing of jQuery was a barrier to adopting it in Drupal, I engaged with the jQuery developers and arranged a call between John Resig and Steven Wittens that quickly cleared the way. Similarly, Drupal's engagement with multiple candidate WYSIWYG editor projects helped spur improvements in the softwares.
2. Fork the Airbnb standard, renaming it. Potentially, we get the full benefit of the standard, plus the ability to tweak as needed, while still pulling in upstream changes as desired. Optionally, we could engage with potentially allied free software projects to do a shared fork. While we acknowledge provenance, we use our own naming and, in our documentation, point to our fork.

OK, here is a new package.json that pins to exact versions using 13.0.0. `npm install` installs for me w/o warnings and eslint seems to work properly with the new config.

Two questions:

- Do we want to shrinkwrap this?
- Do we want to add /core/node_modules to the global .gitignore?

Both should probably be done, but not sure if they are in scope for this issue.

+++ b/core/package.json
@@ -6,13 +6,17 @@
+    "lint:core-js": "node ./node_modules/eslint/bin/eslint.js --ext=.es6.js . --fix || exit 0"

I'd keep "lint:js". It's more predictable and consistency.

Random b/c #2828559: UpdatePathTestBase tests randomly failing.

I'll wait a day or two to get consolidated feedback on what we want in this, and then post a final version for review and hopefully RTBC.

+++ b/core/package.json
@@ -6,13 +6,17 @@
+    "eslint-plugin-react": "6.8.0",

I meant to point this out earlier, and remove in my own patch, but I don't think we need this.

According to https://www.npmjs.com/package/eslint-config-airbnb, it does require eslint-plugin-react and eslint-plugin-jsx-a11y. However, looks like there's https://www.npmjs.com/package/eslint-config-airbnb-base which does not.

Fork the Airbnb standard, renaming it

Per #34.2, we might decide to essentially fork at some point anyway. I'm curious to hear from folks here on what you envision an implementation of such a fork as being. Just inline customizations to our .eslintrc.json file? Or would it be beneficial to publish a full-on fork ala https://github.com/airbnb/javascript/issues/263#issuecomment-146388087?

Should we create another issue for this topic?

Does Drupal forked Symfony packages? I wonder how many packages in Drupal is branded and maintained by a private company like Airbnb or even one person branding.

Symfony packages vs Node packages

For example, the ZendFramework.

"Zend"
It's the same issue. Doesn't it?

I have the same question as @effulgentsia #53. So we forked and :s/Airbnb/Drupal/? Still fork it if we make Zero changes to rules?

@effulgentsia

Since AirBnB does use react, and their guide lines do provide recommendations for JSX/React, if you are using those in your project you might need it. I'll test to see if we actually need them. Additionally I'd strongly urge us to not fork the standard just to have our own copy. We'll always be behind upstream changes. Additionally we can always overwrite rules in our own config like we are doing currently.

Thanks for further comments regarding the proposal to fork the Airbnb standard. Some clarifications.

I wonder how many packages in Drupal is branded and maintained by a private company like Airbnb or even one person branding ... "Zend" It's the same issue. Doesn't it?

The comments I've made here, such as, "When we adopt a standard produced by an entity, ... [w]e are saying: when we do X, we look to Y as an authoritative source, and we encourage others to do likewise", are specific to standards as distinct from software packages.

Similarly, in noting "there's a tension between the cooperative, open internet of free software, on the one hand, and the internet as a set of increasingly powerful, private, proprietary platforms," I'm drawing a distinction between different classes of corporations. There are many companies and individuals that don't present the particular issues of Airbnb.

I'd strongly urge us to not fork the standard just to have our own copy. We'll always be behind upstream changes.

As noted in #34, the proposal is to adopt a particular version of the Airbnb standard: "Any future changes that AirBnb makes to that guide will require a new coding standards issue for Drupal to update to that version." So, either way, we'll be behind upstream changes. Are there specific ways that a fork would qualitatively alter the workflow or increase workload?

Forking seems to be the norm actually - see https://github.com/airbnb/javascript#in-the-wild. Many of the links there are actual forks.

Beyond that, I read this issue and am not sure what it would mean to adopt these standards as a "baseline" - how exactly are they going to be merged with the existing ones at https://www.drupal.org/docs/develop/standards/javascript/javascript-codi...?

1. There are things in the existing standards that aren't covered by the Airbnb ones (for example, https://www.drupal.org/docs/develop/standards/javascript/javascript-codi... and https://www.drupal.org/docs/develop/standards/javascript/javascript-codi...).
2. The Airbnb standards seem to assume everything is written in ES6 (for example, prefer-const and comma-dangle), but Drupal clearly still needs its standards to address ES5 code also.
3. Some of the Airbnb standards appear to contradict existing Drupal standards (that may have particular reasons behind them), but most haven't been discussed here at all, for example:
   1. Having different standards for writing multi-line comments and single line comments seems to go against pretty much how all Drupal code comments are written.
   2. https://github.com/airbnb/javascript#variables--define-where-used appears to contradict https://www.drupal.org/docs/develop/standards/javascript/javascript-codi... in terms of whether variables should typically be defined at the top of a function or not.
   3. https://github.com/airbnb/javascript#blocks--cuddled-elses contradicts Drupal's normal if/else braces style. (This one was discussed above already, but it seems really weird to me that we would deliberately introduce an inconsistency between PHP and JavaScript coding standards like that, especially these days when so many people are writing both PHP and JavaScript at the same time.)
4. I'm not sure https://github.com/airbnb/javascript#jquery--find is a good idea (or at least not always a good idea) based on what I've always heard about jQuery performance, e.g. https://learn.jquery.com/performance/optimize-selectors/#id-based-selectors.

If the actual goal of this issue is to enable #2809281: Use ES6 for core JavaScript development, wouldn't it be better to talk about adopting the ES6-specific standards only (i.e. the ones at https://github.com/airbnb/javascript#ecmascript-6-es-2015-styles) first? Then it could just be a simple addition to the existing Drupal standards for now, e.g. "if you're writing ES6 code follow these particular standards for the new ES6 features".

Also, putting on my core committer hat for a minute, I think this was discussed among Drupal 8 committers only (not Drupal 7). In principle I don't see a fundamental problem with the overall coding standard changes proposed here, but it's worth noting that since #2809281: Use ES6 for core JavaScript development doesn't seem likely to happen for Drupal 7, this issue as written is somewhat "all pain and no gain" for Drupal 7, and we don't have .eslintrc to help in Drupal 7 either (although in principle it could be added).

Again, not a blocker, but if this issue were to only adopt the ES6 specific standards instead (as I mentioned in my above comment) it would be a much cleaner separation.

There are things in the existing standards that aren't covered by the Airbnb ones (for example, https://www.drupal.org/docs/develop/standards/javascript/javascript-codi... and https://www.drupal.org/docs/develop/standards/javascript/javascript-codi...).

In Airbnb Style Guide, Constant is covered by Vars part I think.

If we're going to enhance it, it meant we have to maintain a package for the rule ourselves also (current ESLint can't catch it I think). I think we have the intention to use ESLint to lint our code and skip human scanning at most to max our efforts.

Having different standards for writing multi-line comments and single line comments seems to go against pretty much how all Drupal code comments are written.

Doesn't it same as Drupal?

https://github.com/airbnb/javascript#variables--define-where-used appears to contradict https://www.drupal.org/docs/develop/standards/javascript/javascript-codi... in terms of whether variables should typically be defined at the top of a function or not.

Let & Const are ES6 things. It's block scoped. Above guide's suggestion is reasonable.

I'm not sure https://github.com/airbnb/javascript#jquery--find is a good idea (or at least not always a good idea) based on what I've always heard about jQuery performance, e.g. https://learn.jquery.com/performance/optimize-selectors/#id-based-selectors.

This part we need to benchmark and find out the answer. But I think it's no much diff in jQuery 2.x / 3.x & these days Modern browsers.

In jQuery 1.x, it is handled by Sizzle engine and could be splited, e.g.:

(Close but Not exactly, it's much more conditional)

$('.scoped .child')

// Sizzle 
$('.scoped).find('.child')

// 2.x / 3.x
querySelectorAll('.scoped .child')

** In fact, CORE developers always written $('.scoped .child'). If it's really no helps in 2017, less patch rerolls.

I'm not sure if the coding standards process is broken or if it's just that this particular issue is proposing a lot of changes at once and people weren't aware of that from the earlier issue title. (Personally I was also out-of-town for about 75% of the time during the two week period this was under review, and I didn't read through this issue carefully until I saw it bump up in my issue tracker with lots of other people commenting on it.)

I'm not sure if by "follow airbnb for ES6 code only" you mean only adopting the standards at https://github.com/airbnb/javascript#ecmascript-6-es-2015-styles, or if you mean adopting all the standards but only when writing ES6 code.

Either one of those would address my concerns regarding Drupal 7, however, as it would only apply to code that "opts in" by choosing to depend on ES6. (Coding standards changes actually do apply to all new Drupal code, regardless of whether it's 7 or 8, core or contrib - see the paragraphs at the top of https://www.drupal.org/docs/develop/standards/coding-standards. So straight-up changes to the regular coding standards would affect Drupal 7 code. Whether anyone bothers to update existing, already-written code is a different story.)

On the multi-line comments, it's possible I'm interpreting the Airbnb standards wrong... but the way I read it, it was saying that any time you have a code comment that happens to go beyond one line, you have to switch it over from //-style to /* */-style comments. If so, that would be very different from Drupal's current code comment style.

Attached a patch with my revisions from my above comments.

There's no reason for no-underscore-dangle or consistent-return to be errors or warnings in my opinion as those are purely personal preference. no-plusplus and no-mutable-exports are reasonable as warnings, but again too absolutist for overall coding standards that apply to all of us in my opinion.

I opened #2839733: Decide how to communicate and maintain divergence from airbnb JS styleguide for discussing the forking question. @nedjo: can you add a comment there or update the issue summary there with your thoughts on branding and related concerns?

+++ b/core/package.json
@@ -6,13 +6,17 @@
-    "lint:js": "eslint . || exit 0"
+    "lint:core-js": "node ./node_modules/eslint/bin/eslint.js --ext=.es6.js . --fix || exit 0"

Does this mean we would lose the linting we currently have for non-es6 .js files? If so, that doesn't seem good.

#65,

In the planning, the D8 Core development will not touch non-ES6.js anymore. @see #2809477: Add ES6 to ES5 build process

It's more like going to refactor ALL JS than just unlock ES6 features. (that why this step is important)

More often, code standard is Apple vs Orange. Both sides have their valid opinion. Not longer time you will find a better solution.

On some points, e.g. UPPER CASES for constant. Honestly, I don't know if it's really important. ESLint, JSHints, or JSCS exists for years, but still no rules to catch it. It's unusual. (I hope I didn't miss something)

Technically, we may not see the for-loop, @see https://github.com/airbnb/javascript#iterators-and-generators
"no-plusplus" is less important.

I think the whole Drupal community really have to think about "coding standards process" again. We all great developers here, we understand how important the code standard is. But on another side, we need to prioritize the most important part of your project and find an efficient way to move things forwards.

In the planning, the D8 Core development will not touch non-ES6.js anymore.

Ah, I see. From #2809477: Add ES6 to ES5 build process:

Create a script that rename all the existing JavaScript files to *.es6.js

Which is great for D8 core. But D7 core, and most D7 and D8 contrib, might still choose to use .js rather than .es6.js. And those projects would still follow https://www.drupal.org/docs/develop/standards/javascript/javascript-codi... for their .js files. In which case, how would they lint them? For D7, I suppose it's a non-issue, as #63 is a patch for 8.x only. But for D8 contrib, does this present a problem?

+++ b/.eslintrc.json
@@ -1,3 +1,4 @@
-  "extends": "./core/.eslintrc.json"
+  "extends": "./core/.eslintrc.json",
...
+++ b/core/.eslintrc.json
@@ -1,5 +1,5 @@
-  "extends": "eslint:recommended",
+  "extends": "eslint-config-airbnb",

#67,

And those projects would still follow https://www.drupal.org/docs/develop/standards/javascript/javascript-codi... for their .js files. In which case, how would they lint them?

this applies Core concept and put everything into Core dir. We could make another command or a .example in Root. It shouldn't a blocker for this topic.

Personally, I'd expect we make boilerplates and share it. You should always pick the best one that fit your workflow rather than Core only.

@droplet

Could we move the existing rules to /.eslintrc.json, dropping the root key in the process, and then use the airbnb/our-own-fork in /core/.eslintrc.json?

#69 makes sense to me, in terms of keeping BC for contrib.

It shouldn't a blocker for this topic.

Why not? Currently, contrib is able to eslint their JS files thanks to HEAD's /.eslintrc.json. Per #60.2, this issue is about adopting new standards for core .es6.js files. Therefore, it's out of scope for this issue to break what is already working for contrib .js files, unless there's a technical reason why we must break that. But if #69 can be made to work, I think it's worth it as a way of minimizing disruption to contrib.

Could we move the existing rules to /.eslintrc.json

existing rules? current code standard?

#70,
With the current design, the .js compiled by builder script will NOT follow the code standard. (It's almost impossible to make it 100% compatible.) It creates much noise if we support .js by default.

For contrib, I think opt-in to use old code standard better than opt-out.

On the same way, moving package.json would be more meaningful if we going to provide a command. And we pinned the ESLint to specified version, not using the global one. To avoid these issues: https://www.drupal.org/node/2821113#comment-11742234

Either PHP composer or Node Packages, both recommended to install package locally, doesn't it?

#2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8

How people deal with the D8, D7 and contrib issues in PHP short-array? I think it has same situation? We could learn from it.

#72, not sure it is apples to apples b/c the conversion to ES6 that will happen, but in general if editing existing code, we follow conventions in the file. So, if an old file uses array() syntax, we use that for consistency. Periodically, this gets broken down by method, but the two are never used in the same method. New files use the [] syntax. Similar things happen with how safemarkup gets handled, how phpunit exceptions get handled, not using deprecated functions, and few other things.

This is more or less what I was getting at in #69.

We are nearing code freeze for 8.3.x and this would be ideal to get in before then, so bumping. I can't tell what else is needed before we can mark this RTBC. Tagging for an issue summary update.

This is a blocker for #2809281: Use ES6 for core JavaScript development.

• Fixes an extra comma that was present in #74.
• Updates .eslintrc.json rule overrides with some we'll need.
• Updates our dev dependencies and creates npm-shrinkwrap.json locking down dependency versions.

U+1F44D

Seems like we need some direction from the Technical Working Group in order to move forward here. As pointed out in #76, this is a blocker for moving forward with modernizing JavaScript in Drupal core for 8.4.

The TWG has a meeting on February 14. This is on the list of topics to discuss.

+++ b/core/.eslintrc.json
@@ -18,80 +18,23 @@
+    "no-mutable-exports": [1],
...
+    "no-mutable-exports": [0]

Duplicate key in JSON. Need to pick either warning or turn off completely.

I originally added this as a warning instead of a hard error because it is good style practice to avoid doing this when possible, but it will generate noise in the linter. I'm not sure if a warning would/sould block a core (or potential contrib module's js code).

Fixes the issue in #82.

I think that marking this as a warning makes sense, especially since this this overrides the Airbnb rule.

The TWG has discussed this issue and are working on formulating a response. Expect an update soon.

@tizzo

Any update on this?

• Switch to babel-preset-env.
• last 2 versions, and ie >= 9

This looks good to me, I like it! (Obviously I can't test it with existing files as we don't currently have any .es6.js files, but I made one locally and poked it a bit)

+1

+1

+1

I think this needs to be moved to the Drupal project to test the patch before RTBTC, but we're in limbo because of the TWG.

I updated the issue summary to describe the current approach by drpal.

Do you think this is ready to move into the core issue queue, @webchick, or does this still need to be in the TWG queue? @_nod? @droplet?

+++ b/.eslintignore
@@ -1,5 +1,4 @@
+core/**/*

Checking I understand this correctly - we lose linting of current core JS with this change. Right?

I tried to test the patch in linux, but npm install fails because "Unsupported platform for fsevents@1.1.1", which seems to be a MacOS-only library. This is due to the npm-shrinkwrap.json file, which explicitly defines it.

I'm moving this back to Needs Work because of that. I think build systems will start failing.

@alexpott, the --no-ignore switch to eslint will allow linting core js files, but the npm run-script lint:core-js will no longer lint core js files. It would be a good idea to document this either in core itself or in the docs?

@alexpott

This patch changes the files that should be linted in core *.es6.js. Going forward, the *.js files will just be the built output and not actually edited. A followup to this issue, and a script that is already included, will convert all extensions to .es6.js and then create the es5 .js equivalent files.

Why not to include !core/**/*.es6.js in .eslintignore? Or even simpler: !*.es6.js?

@pingwin4eg

Which .eslintignore? There are now two because there are now two sets of standards. One being applied to contrib, and one being applied to core.

On the package.json github issue que there is a suggestion: fsevents contains "os": ["darwin"] in package.json, which prevents the error from showing up because npm will not even try to install that package and instead it will only print a warning.
I don't know the details, but something like this seems like it could be the solution to #94.

@drpal so there'll be a period of time when there's no lint coverage of core javascript from the testbot. Why can't we add a second ruleset for es6 and once all of core's js is build using es6 move to that being the default? There's likely to be sometime after committing this patch before we get to building all core js from es6 no?

@drpal Sorry, I didn't notice that there will be 2.

In that case I guess the testbot can run 2 separate commands - the one for current standards and 2nd for es6 files. This can be done by specifying eslint command-line options AFAIK. Is that possible for the testbot?

Ultimately, after all core js will be doubled with es6 copies, we can leave current eslintrc and ignore in drupal root dir, and provide new ones for *.es6.js in core/ dir. This way only the core's .eslintignore will be used for core, and as rulesets have inheriting behaviour, they can be fully overridden in core/.eslintrc.json.

@pingwin4eg

Ultimately, after all core js will be doubled with es6 copies,

Yes, that's correct.

we can leave current eslintrc and ignore in drupal root dir, and provide new ones for *.es6.js in core/ dir.

The reason for two .eslintrc files is that contrib can keep the current rules, and core can have the new rules based off of the AirBnB standards. We also have two .eslintignore for handling different ignore cases.

@mradcliffe

I've removed the shrinkwrap file. I think this may solve our issue, and I feel its pretty safe since we are using exact versions in package.json, no hats.

Additionally, since we are already using sourcemaps, does it make sense for us to minify the built es5 files? Babili could be added as a Babel preset quite easily and would offer us nicely minified files from the start.

@mradcliffe

For reference I just tested this on a fresh instance of Ubuntu 16.04.1 LTS, Node 7.7.4 and it installs correctly. U+1F44D.

Awesome, thanks, @drpal! I was able to apply the patch, run npm install on Linux 3.18, Mac OS and Windows 7. I did not run into any further issues.

+++ b/2815077-86.patch
@@ -0,0 +1,2893 @@
+ ¶
...
+ ¶

I applied the patch and got some whitespace warnings. This can probably be fixed on commit.

<stdin>:2999: trailing whitespace.
<stdin>:3011: trailing whitespace.
<stdin>:3011: new blank line at EOF.

I updated the issue summary with this and @alexpott's question in #99.

@mradcliffe

My fault on this one. I accidently included a patch in the last patch.

Additionally, #2818825: Rename all JS files to *.es6.js and compile them has been updated to include the duplication of .js files to .es6.js. That patch should be committed first.

That looks great. Thank you for clarifying #2818825: Rename all JS files to *.es6.js and compile them , which I think sort of answers @alexpott's question about coverage. If that issue is committed first, then there won't be any lost coverage unless code is committed to a .js file and not the .es6.js files. This might be something that happens though any js commits should probably get a bit of extra scrutiny until they are removed.

Is there a follow-up issue on that as well? I'm a little lost finding it.

@mradcliffe

I assume you mean a followup issue about the changes to JS workflow for core? There is the parent meta issue, but nothing specifically around that. If you want to file one that would be wonderful.

Sorry. I think it's over complicated! The current patch & way will break a lot of editors' built-in inline ESLint feature (e.g. PHPStorm, VSCode.) Wrong config in .eslintignore I think.

I never expected we need our developers to run `npm run lint:core-js` in CORE DIR manually every time when they code JS. Also, don't want to see anywhere have articles teaching us the basic JS stuff for Drupal Core/Contrib Development. (No Drupalism Mode, please.)

- Why don't let Contrib opt-in to use old JS Code Style? Just copy the old config to their module root. I think this little action is acceptable.

- Do we promote to use old JS Code Standard in Contrib forever? When to stop it? And why do you create 2 noises for our developers but not just do it once now?

Anyway, shouldn't we focus on "CODE STYLE" rather tooling on this issue thread?

Sorry. I think it's over complicated!

Alright.

The current patch & way will break a lot of editors' built-in inline ESLint feature (e.g. PHPStorm, VSCode.) Wrong config in .eslintignore I think.

You do have a point. I missed adding "root": true to both .eslintrc.json files, which I just added in this patch. This will prevent rules from parent folders leaking into the current workspace.

- Why don't let Contrib opt-in to use old JS Code Style? Just copy the old config to their module root. I think this little action is acceptable.

I really want to avoid contrib having to change their workflow just to get stuff done.

- Do we promote to use old JS Code Standard in Contrib forever? When to stop it? And why do you create 2 noises for our developers but not just do it once now?

I don't know how to handle this, but it's also not within the scope of this issue.

Anyway, shouldn't we focus on "CODE STYLE" rather tooling on this issue thread?

I think what may be left, is to review the overrides from the AirBnB standards.

Removing the babel-preset-env portion of this patch. I'll create another issue with that and some additional changes. Created, #2868137: Improve JavaScript build tooling.

Thanks @drpal.

Remaining questions:

• Quoted #92

  I think this needs to be moved to the Drupal project to test the patch before RTBTC, but we're in limbo because of the TWG.

  I updated the issue summary to describe the current approach by drpal.

  Do you think this is ready to move into the core issue queue, @webchick, or does this still need to be in the TWG queue? @_nod? @droplet?

• Do we promote to use old JS Code Standard in Contrib forever? When to stop it? and How?

I read a letter from 52 great Drupal Developers today. I hope we have a better transparent discussion.

This is high priority for the TWG and it will certainly be discussed during the next meeting. Unfortunately the last couple of meetings were cancelled because it wasn't possible for everyone to attend due to scheduling conflicts in real life.

I discussed this issue just now with @tizzo and @pfrenssen on a TWG / Coding Standards call, and will post a longer comment with our thoughts within the next day or so.

Assigning to myself per #114.

Thank you for everyone's patience on this, and I apologize for all the delays this issue has had. Though it might not seem that way given all the delays, I'm hugely in favor of updating core to use ES6, and I very much appreciate everyone's work towards that, both on this issue, and on all the other parts of #2809281: Use ES6 for core JavaScript development.

Back in #70, I argued for needing to preserve BC for D8 contrib projects that choose to not adopt ES6, and #111 implements that BC by moving the old (non-airbnb) standards to the root .eslintrc.json file. Thank you for that.

However, in #71 and #109, @droplet argued for having a single JS coding standard for Drupal 8, and requiring contrib projects that don't want to follow it to have to explicitly opt out of it.

On our last TWG call (per #114), @tizzo and @pfrenssen found @droplet's position more convincing and they convinced me to go along with it. I'm therefore retitling this issue accordingly.

The last patch to implement that approach was #63. However, I'm happy with the work that's been done since then, because I don't think #63 made it easy enough for contrib projects to opt in to the old standards (e.g., as a temporary measure until they're ready to adopt the new ones). With that patch, each contrib project needing that temporary coding standards BC would need to duplicate its own full copy of the legacy .eslintrc.json file.

But I think #111 only needs to be slightly tweaked to address that. Here's a patch that starts with #111, reverts the root-level .eslintrc.json file to what was in the #63 patch, and renames #111's root-level .eslintrc.json to .eslintrc.legacy.json. This allows a contrib project to opt into the legacy rules by just adding the following .eslintrc.json to its root:

{
  "extends": "../../.eslintrc.legacy.json",
  "root": true
}

That relative path assumes that the project is in the /modules directory, rather than nested deeper into /sites or /profiles, but I think that's an ok assumption for the .eslintrc.json committed to that project's repository. A local checkout that places the project elsewhere, and wants to eslint it, can locally modify that relative path. Perhaps a follow-up improvement can be to publish both the new and the legacy standards to named packages, so that they can be referenced both within Drupal projects and non-Drupal projects without resorting to relative paths. Let's discuss that in #2839733: Decide how to communicate and maintain divergence from airbnb JS styleguide .

The last time the TWG put out a call for final discussion on this issue was in #26. However, comments #60 through #62 demonstrate that it was unclear as to what the scope of this issue is. For example, is D7 included in the change or not? And is D8 contrib included in the change or not? Therefore, I think we'll want to clarify that in the issue summary and re-announce a final discussion round. Tagging accordingly. However, I hope the patch itself can continue to make progress in the meantime. For example, any feedback about my proposed change in this increment?

Additionally:

+++ b/core/.eslintrc.json
@@ -18,80 +19,23 @@
+    "consistent-return": [0],
+    "no-underscore-dangle": [0],
     "max-nested-callbacks": [1, 3],
+    "no-mutable-exports": [1],
+    "no-plusplus": [1, {
+      "allowForLoopAfterthoughts": true
+    }],
+    "no-param-reassign": [0],
+    "no-prototype-builtins": [0],
     "valid-jsdoc": [1, {
       "prefer": {
         "returns": "return",
         "property": "prop"
       },
       "requireReturn": false
-    }]
+    }],
+    "brace-style": ["error", "stroustrup"],
+    "no-unused-vars": [1]

Can we have inline comments for why we're making these deviations from airbnb?

I updated the issue summary.

Thank you, @effulgentsia, @tizzo and @pfrenssen.

Can we have inline comments for why we're making these deviations from airbnb?

We could do this if switched to YAML instead of JSON.

This is a patch that changes to YAML in order to add comments. The comments that are there need improvement in my opinion, but I didn't really have much time to think those through.

This does increase the patch size and file length of the config files. It probably slows down eslint, but I didn't see anything noticeable when I ran it on core (with --no-ignore so I could see it parse through all the issues with the

But I think #111 only needs to be slightly tweaked to address that. Here's a patch that starts with #111, reverts the root-level .eslintrc.json file to what was in the #63 patch, and renames #111's root-level .eslintrc.json to .eslintrc.legacy.json. This allows a contrib project to opt into the legacy rules by just adding the following .eslintrc.json to its root:

I don't really see the point in doing this. All we are doing is making a slightly more confusing system for developers of contrib modules. If we want to keep the legacy rules available for them, let's create a timeline for when they will be removed and the new AirBnB rules will be implemented for everyone.

Additionally, since the root .eslintrc.json is extending the on in core, we are going to need to move the package.json and associated node_modules up to the root of the project to support the extension of the AirBnB standards.

We could do this if switched to YAML instead of JSON.

I don't care either way, but this is probably not the issue for switching from JSON to YAML. The overrides to the AirBnB standards should be documented some place I agree.

I wonder if anyone uses a package.json in the root directory of their Drupal install already?

But right, without package.json in the root directory, then the only way to run on modules is

./core/node_modules/.bin/eslint **/*.es6.js (if cwd is drupal root).

This does work.

If cwd, say in an IDE, is the module path, then ../../core/node_modules/.bin/eslint **/*.es6.js works as well.

Alright. These changes should reflect the meeting nodes from DrupalCon Baltimore. Additionally this is a diff between #111 and this patch.

I realized I left out the requested Yarn lock file.

I was successfully able to npm install on MacOS, .

I tested running the npm run-script lint:core-js command within core directory, and then manually ran linter on my contrib module to look at all the fails using ./core/node_modules/.bin/eslint --ext=.js modules/my_module/js. These instructions should go in the published Change Record for contrib, custom and core developers.

Potential Instructions for front end developers.

- Core developers: cd core && npm run-script lint:core-js
- Contrib developers using new standards: ./core/node_modules/eslint/.bin/eslint.js --config=./core/.eslintrc.json --ext=.es6.js modules/my_module/js
- Contrib developers using legacy standards: ./core/node_modules/eslint/.bin/eslint.js --config=./core/.eslintrc.legacy.json --ext=.js modules/my_module/js.

I talked with @drpal at Baltimore and he filled me in on the working call that happened earlier. The explanations/YAML can come in a follow-up.

Change record is already done. Whoops. Thanks, drpal.

Moving over to the core issue to see if there are any patch fails.

Re-organized tasks.

+++ b/core/.eslintrc.json
@@ -18,80 +19,23 @@
+    "brace-style": ["error", "stroustrup"],
+    "no-unused-vars": [1]

This 2 diff to legacy config. any reason?

Only the brace style has a solid reason. (following PHP Standard). Other rules just apple and orange.

JSON format is more friendly than YAML for JS Developers I think. Actually, the ESLint JSON config is supported comments.

"brace-style": ["error", "stroustrup"]

I think this makes sense to keep.

"no-unused-vars": [1]

I want to keep this for now since we already have some code that contains unused variables.

JSON format is more friendly than YAML for JS Developers I think. Actually, the ESLint JSON config is supported comments.

Despite whatever ESLint might be doing, comments are not allowed in the JSON spec. I don't really have an opinion on switching to YAML other than it shouldn't be done in this issue.

@drpal,

+++ b/core/.eslintrc.json
@@ -18,80 +19,23 @@
-    "brace-style": [2, "stroustrup", {"allowSingleLine": true}],
...
-    "no-unused-vars": [2, {"vars": "all", "args": "none"}],

Do you think this is non-sense (not enough, or whatever reason) and update it to current config, or misreading?

It makes sense to drop `allowSingleLine` in `brace-style`.

But "no-unused-vars": [2, {"vars": "all", "args": "none"}] is better than just a warning I think.

@droplet

Are you ok with moving the discussion about refactoring the overrides to an additional discussion? I think it's something that can happen after this is implemented without too much difficulty.

Fail was b/c https://www.drupal.org/node/2863416#comment-12062889 so it looks like we are still RTBC.

I updated the change record based on my comments above in #123 for the various types of linting that a core or non-core developer would do.

The change record may need to be further refined after there are es6.js files in core and whether or not contrib will use that convention or not.

I'm planning to commit this to 8.4.x tomorrow morning (UTC).

I credited @nod_, @droplet, myself, @nedjo, @tizzo, @David_Rothstein and @pfrenssen for comments that helped scope this issue and build consensus. Thanks everyone. See you all in the many followups.

Was just about to commit this and noticed "needs announcement for final discussion" tag. I think we should commit and announce but that is not my call. I'm asking the technical working group for guidance. It seems strange because we already have the "Approved Coding Standard" tag too.

@tizzo contacted me and said it was okay to commit this and announcing later.

git commit -m 'Issue #2815077 by drpal, mradcliffe, effulgentsia, mpdonadio, nod_, droplet, alexpott, nedjo, tizzo, David_Rothstein, pfrenssen: Adopt airbnb javascript style guide v13 as new baseline javascript coding standards for Drupal 8 core and contrib'

I changed the status of the Change Record to published as this is committed.

+++ b/core/package.json
@@ -6,14 +6,18 @@
+    "eslint-config-airbnb": "14.1.0",
...
+    "eslint-plugin-jsx-a11y": "4.0.0",
+    "eslint-plugin-react": "6.10.3",

this is outdated already! and doesn't match the issue title.

@droplet
Please create a new issue :)

@droplet

Created #2878620: Update AirBnB Coding standards to most recent major versions.

Updated CR title to match the patch.

I was looking forward to playing with this so I have been trying out the instructions in the change record [#2873849]. I have never done any JS linting so I am a good guinea pig to check if the instructions are clear to follow for everyone.

Here are my results:

1. Core - ECMAScript 5 (legacy)
   These instructions don't seem to work out of the box:

   $ cd core/
   $ ./node_modules/.bin/eslint --config=.eslintrc.legacy.json --ext=.js .
   

   Nothing seems to happen. To get results I need to remove the line *.js from .eslintignore.

2. Core - ECMAScript 6
   This works fine. I renamed a bunch of JS files in the misc/ folder to *.es6.js and I get back results:

   $ cd core/
   $ rename .js .es6.js misc/*
   $ yarn run lint:core-js
   ...
   357 problems (158 errors, 199 warnings)
   

3. Non-Core - ECMAScript 5 (legacy)

   These instructions are not clear. I was trying this out on the Panels module which has a bunch of legacy JS files in the folder panels_ipe/js.

   The instructions suggest to extend /core/.eslintrc.legacy.json in the module's .eslintrc.json file and specify root: true. So this is the file I made:

   {
     "extends": "/core/.eslintrc.legacy.json",
     "root": true
   }
   

   It is not explained how to run the command but I try it from the module folder:

   $ cd modules/contrib/panels
   $ ../../../core/node_modules/.bin/eslint --ext=.js .
   Cannot read config file: /core/.eslintrc.legacy.json
   Error: ENOENT: no such file or directory, open '/core/.eslintrc.legacy.json'
   

   OK so I hardcode the relative path in the file, but this doesn't seem like a good idea since this file will be committed in the project's repository. The module can be installed in modules/ but also in arbitrary subfolders like modules/contrib/. The relative path will not work in all cases.

   This is now my .eslintrc.js file:

   {
     "extends": "../../../core/.eslintrc.legacy.json",
     "root": true
   }
   

   I try the command again:

   $ cd modules/contrib/panels
   $ ../../../core/node_modules/.bin/eslint --ext=.js .
   

   Nothing happens. The files do not seem to be scanned. Even when I edit the core/.eslintignore file to remove the line *.js nothing happens.

   This section could use some more details on how to get it working.

4. Non-Core - ECMAScript 6
   First off there is a typo in the filename here, it says eslintrc.json without the leading period.

   This is my .eslintrc.json file:

   {
     "extends": "../../../core/.eslintrc.json",
     "root": true
   }
   

   I try it out after renaming the JS files to .es6.js:

   $ cd modules/contrib/panels
   $ rename .js .es6.js panels_ipe/js/*
   $ ../../../core/node_modules/.bin/eslint --ext=.es6.js .
   ...
   130 problems (121 errors, 9 warnings)
   

   Works great!

So my results were mixed :) The legacy instructions seem to need a bit of attention.

@pfrenssen so the core legacy scenario (1) is not really relevant the moment #2818825: Rename all JS files to *.es6.js and compile them lands there is no legacy javascript in core.
I can get non core legacy to wrk by just doing:
../../core/node_modules/.bin/eslint -c ../../core/.eslintrc.legacy.json --ext=.js .

My panels was at modules/panels

@pfrenssen actually one more thing needed for it to work - no copying or extending needed...
If panels is at modules/panels.
../../core/node_modules/.bin/eslint --no-eslintrc -c=../../core/.eslintrc.legacy.json --ext=.js .

@pfrenssen I've updated the CR so that all of the scenarios provided there should work.

Great, thanks Alex!

Before starting, ensure that you are using at least the latest LTS release of Node.js. If you expect you may want to install newer versions of Node.js, NVM is recommended.

We need a better suggestion here. Don't we lock it to any Node version? NVM is a headache to non-NODEJS developers I think. Especially on Windows.

@droplet

Created #2879190: Specify a range of supported Node.js versions in package.json to specify our supported node.js versions.

While change records happened, drupal.org docs updates did not. Opened #2888877: [PP-2] Update documentation following airbnb javascript style guide v13 adoption for that. Removing the tag.

Title update to make it reflect what actually got committed.

removing tag.