Problem/Motivation
We're going to use the newer version of JS (ES6) in core #2809281: Use ES6 for core JavaScript development, that comes with some pretty significant language improvements that we need to have standards for. Instead of making up our own we should make use of an existing established standard close to ours.
One of the most well known and used standard similar to ours is airbnb JavaScript Style Guide. They have rules for new ES6 features, they also use react hence have react rules ready. Won't help core but might be helpful to contrib and projects. Their rules are more strict than ours, it's a good move for our codebase.
This reasoning is continued in the original Issue summary section below.
Some projects using this standard:
- React
- Evernote
And it seems ember.js coding standards are extremely similar.
Proposed resolution
This is the current proposed resolution/patch:
1. Adopt the Airbnb JavaScript style guide with noted exceptions for core ES6 JavaScript files (#32 mradcliffe, #66 drpal).
2. Allow contrib modules to opt out by specifying legacy rules, but strongly encourage/enforce the modified Airbnb JavaScript style guide.
3. Allow core non-ES6 JavaScript files to use the legacy eslint rules with small tweaks (#69 drpal, #71, #109 droplet, #111 effulgentsia).
Question: "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?"
The plan is to duplicate/rename files in #2818825: Rename all JS files to *.es6.js and compile them which should be committed first before this patch.
Exceptions to Airbnb JavaScript style guide
- Allow without warning inconsistent (early) return.
- Allow without warning underscore dangle.
- Allow with warning exceeding maximum nested callbacks.
- Allow with warning use of ++ operator
- Allow without warning parameter re-assignment.
- Allow without warning prototype built-ins.
- Do not allow brace style "stroustrup"
- Allow with warning unused vars
Remaining tasks
- Improve documentation around exceptions to Airbnb JavaScript style guide. This should be done in #2888877: [PP-2] Update documentation following airbnb javascript style guide v13 adoption as per working group call at Baltimore.
- Send out another announcement.
Follow-up:
- Work on #2818825: Rename all JS files to *.es6.js and compile them after this issue is committed per working group call at Baltimore.
Original report by nod_
The only significant whitespace change that will impact us are the config for brace-style
and object-curly-spacing
.
Current drupal style
if (this.model.get('isActive')) {
// …
this.model.set({isActive: true, activeTour: $tour});
// …
}
else {
// …
}
Airbnb style
if (this.model.get('isActive')) {
// …
this.model.set({ isActive: true, activeTour: $tour });
// …
} else {
// …
}
People might complain but it's a really minor thing in the grand scheme of things. And if we really have to, we can override those 2 rules. The rest of the rules enforce good practices.
Right now running coding standard on our codebase, after eslint auto-fix of some rules (and without the whitepace rules discussed above) there are 2061 problems, 95% of which come from the following rules:
"camelcase"
"no-param-reassign"
"func-names"
"comma-dangle"
"max-len"
"no-underscore-dangle"
"no-unused-vars"
"no-plusplus"
"newline-per-chained-call"
"no-restricted-syntax"
"new-cap"
"no-new"
"padded-blocks"
"prefer-const"
"no-use-before-define"
"consistent-return"
"no-prototype-builtins"
"no-else-return"
"default-case"
"no-shadow"
Attaching a patch since adopting this means changing some eslint files.
Previous discussions
#1778828-51: [policy, no patch] Update JS coding standards
#1778828-59: [policy, no patch] Update JS coding standards
#2746807-2: Consider standard js coding standard
Comment | File | Size | Author |
---|---|---|---|
#122 | interdiff-121-122.txt | 79.81 KB | GrandmaGlassesRopeMan |
#122 | 2815077-122.patch | 86.75 KB | GrandmaGlassesRopeMan |
#121 | interdiff.txt | 5.68 KB | GrandmaGlassesRopeMan |
#121 | 2815077-121.patch | 7.39 KB | GrandmaGlassesRopeMan |
#118 | 2815077-118.patch | 8.87 KB | mradcliffe |
Comments
Comment #2
nod_(patch to apply after #2809477: Add ES6 to ES5 build process)
Comment #3
nod_Comment #4
nod_Comment #5
martin107 CreditAttribution: martin107 as a volunteer commented+1 ... using a standard followed by React is the key argument.
Comment #6
mpdonadioWhile 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).
Comment #7
nod_In the age of IDE and auto format tools I don't feel it's a big deal even if it's different from PHP. Different languages and all.
At this level whitespace is a detail (nobody talked about using tabs or having an indentation of 3). I'm more interested in opinions about the rest this'll impact for people. Like is the documentation on this page https://github.com/airbnb/javascript good enough, can we just replace our standard page with this or do we have to massage the writing a bit more so it's less dry? that kind of stuff.
Comment #8
mpdonadio#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.
Comment #9
droplet CreditAttribution: droplet commentedThe 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
Comment #10
nod_Not a great idea to not do local linting but testbot will run eslint for all patches "soon" #2600626: [PP-1] Ensure availability of node/npm in the testrunners.
Fair question. Here is some data, the up to date list of people commenting on 8.x js issues (pay attention to the names in bold only, those are people that recently commented on issues). Most of those don't write patches or reviews but create the issues.
We had a meeting about JS at drupalcon with core committers, they run eslint before committing a patch. From the top 20 people commenting on 8.1+ JS issues: here is the list (and # of comments), of people that would either write JS or review it: nod_ (1288), WimLeers (806), droplet (555), andypost (140), tedbow (115). We also have many infrequent contributors that I know run eslint on their patches already.
The patch review logjam is already happening and this will have no impact on it since the people doing the most reviewing are the one to push for this. Having a stricter coding standard is going to help actually. JS bugs come from bad scoping, using references when not expected and so on, an else on the same line as } won't trip us up.
Comment #11
dawehnerThe way how we solved / still solve it for php is the following:
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?
Comment #12
nod_eslint can fix many rules (not all of them). We can always contribute upstream if we care enough about a rule to auto-fix it's not too hard. jscodeshift is more for big bang changes, at least that's how I see it for us.
We already went through hell for the first round of jshint/eslint changes, we'll be more efficient this time around.
Comment #13
droplet CreditAttribution: droplet commentedJust 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.
Comment #14
droplet CreditAttribution: droplet commentedData 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.
Comment #15
nod_I think jscs takes care of func-name.
The param one is a problem but would be good to follow that one down the line.
Comment #16
nod_Beside working out how to follow this one no-param-reassign rule we agree on the plan. so tagging for discussion.
Comment #17
droplet CreditAttribution: droplet commentedThis 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.
Comment #18
nod_More than 2 week without any kind of feedback, what's going on? All our nice momentum from last month died out because we can't start the conversion until we have the coding standard. And since this impact all 8.x JS patches, of which there are more than 200, this is really important.
Please let us know what are the next step here. I checked the project workflow we're stuck at step 3. there are 9 steps.
Comment #19
pfrenssen@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.
Comment #20
nod_Thank you, a comment saying it's been added to the list would have been nice. Wouldn't have complained as much :)
Comment #21
alexpottI'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.
Comment #22
alexpottShould 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?
Comment #23
fafniricalIt's outdated now. eslint-config-airbnb v13.0.0 was released yesterday. A package.json file with the latest versions would have:
Comment #24
droplet CreditAttribution: droplet commentedpardon, 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.
Comment #25
alexpott@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.
Comment #26
alexpottThis got announced for final discussion by the TWG - https://groups.drupal.org/node/515644.
Comment #27
tizzo CreditAttribution: tizzo commented@alexpott - Appreciate your pointing that out - forgot to update the tag. Thanks.
Comment #28
tizzo CreditAttribution: tizzo commentedThis recommendation is approved by the coding standards committee on December 20, 2016.
Comment #29
effulgentsia CreditAttribution: effulgentsia at Acquia commentedMoving 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.
Comment #30
effulgentsia CreditAttribution: effulgentsia at Acquia commentedMoving to the actual core queue :)
Comment #31
alexpott@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.
Comment #32
mradcliffeIs 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?
Comment #33
nedjoUpdating 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.
Comment #34
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNot a full answer to #33, but for now...
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:
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.
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?
Comment #35
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRefining the issue title further
Comment #36
nedjoSure.
One of our Drupal principles (disclosure, I wrote the first version of them) is:
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."
There is of course no shortage of Javascript coding standards or style guides. Without suggesting they're necessarily appropriate, a few are:
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.
Comment #37
droplet CreditAttribution: droplet commentedIt'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
Comment #38
nod_The airbnb standard is the only one defining standards for most if not all ES6 constructs. This is critical for us since a coding standard is the only thing we're waiting on to actually use ES6 in drupal core: #2809281: Use ES6 for core JavaScript development. There are not enough JS people in the core queue to maintain or even define our own thing. The other standards out there are not as good a fit, this one is the most extensive, documented, and as a bonus it's very close to our current code.
Don't agree 100% with that. Promoting, maybe, endorsing not really. Few years ago the most accessible video player ever was made by paypal, open source and all. Using it doesn't mean endorsing paypal… Recognizing their contribution to open-source, sure. I'm using PDF not because I want adobe to succeed but because I don't want to have a miserable time sending formatted document to clients.
Comment #39
mradcliffeI 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.Comment #40
20th CreditAttribution: 20th commentedI 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.
Comment #41
GrandmaGlassesRopeMan@nod_
I rerolled this. Additionally I'm not sure we need to include
eslint-plugin-react
since I don't believe we'll be supportingJSX
anytime soon.Comment #42
mpdonadioPer #34, don't we need to pin this to exactly 13.0.0?
Comment #43
GrandmaGlassesRopeMan@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.Comment #44
mpdonadio#43, I should have taken more time with my comment. In #34, there was:
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.
Comment #45
nedjoHere's another way of putting the contrast I made above between the open internet of free software and proprietary platforms like Airbnb:
That's from a blog post earlier this year by Dries Buytaert.
Okay, I acknowledge a bunch of good points made, including:
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.
Comment #46
mpdonadioOK, 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.
Comment #47
droplet CreditAttribution: droplet commentedI'd keep "lint:js". It's more predictable and consistency.
Comment #49
mpdonadioRandom 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.
Comment #51
GrandmaGlassesRopeManI meant to point this out earlier, and remove in my own patch, but I don't think we need this.
Comment #52
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAccording to https://www.npmjs.com/package/eslint-config-airbnb, it does require
eslint-plugin-react
andeslint-plugin-jsx-a11y
. However, looks like there's https://www.npmjs.com/package/eslint-config-airbnb-base which does not.Comment #53
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPer #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?Comment #54
droplet CreditAttribution: droplet commentedShould 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?
Comment #55
GrandmaGlassesRopeMan@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.
Comment #56
nedjoThanks for further comments regarding the proposal to fork the Airbnb standard. Some clarifications.
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.
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?
Comment #57
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedForking 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...?
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".
Comment #58
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedAlso, 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.
Comment #59
droplet CreditAttribution: droplet commentedIn 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.
Doesn't it same as Drupal?
Let & Const are ES6 things. It's block scoped. Above guide's suggestion is reasonable.
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)
** In fact, CORE developers always written
$('.scoped .child')
. If it's really no helps in 2017, less patch rerolls.Comment #60
nod_So can we take a second and acknowledge that the coding standard process broke down and need to be fixed? We followed the steps, waited 2 months for a decision and when an official one was made, people come out of the woodwork to contest it. Not saying there are no valid questions raised but it's starting to look like a core UX issue.
Anyway, let's assume we fork it to go around the naming issue.
#57:
eslint --ext=.es6.js .
, doing just that in essence.Maybe it wasn't obvious but we've never planned on touching D7. The D7 js will be touched as little as possible and it's pointless to change the standard at this point in time since there won't be ES6 in D7 core js.
Comment #61
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI'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.Comment #62
nod_"all the standards but only when writing ES6 code" <= That one.
I think there are few enough js changes to D7 so that it's doesn't matter much. Even with a new standard I'd say consistency wins over standard conformity. Since there is no plan to update D7 js, it would stay mostly the same.
On the multi line comment it's ambiguous enough to have several interpretations, so we pick whichever suits us. And if we fork we can add a sentence to make it more precise.
On the timing it's too bad because we could have discussed all this earlier, outside of the holidays if the different announcements had been made earlier. The momentum we had 2 months ago is gone and addressing the issues raised would have been smoother then than now.
Comment #63
mradcliffeAttached 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.
Comment #64
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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?
Comment #65
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDoes this mean we would lose the linting we currently have for non-es6 .js files? If so, that doesn't seem good.
Comment #66
droplet CreditAttribution: droplet commented#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.
Comment #67
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAh, I see. From #2809477: Add ES6 to ES5 build process:
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?Comment #68
droplet CreditAttribution: droplet commented#67,
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.
Comment #69
GrandmaGlassesRopeMan@droplet
Could we move the existing rules to
/.eslintrc.json
, dropping theroot
key in the process, and then use the airbnb/our-own-fork in/core/.eslintrc.json
?Comment #70
effulgentsia CreditAttribution: effulgentsia at Acquia commented#69 makes sense to me, in terms of keeping BC for contrib.
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.Comment #71
droplet CreditAttribution: droplet commentedexisting 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?
Comment #72
droplet CreditAttribution: droplet commented#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.
Comment #73
mpdonadio#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.
Comment #74
GrandmaGlassesRopeManThis is more or less what I was getting at in #69.
Comment #75
webchickWe 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.
Comment #76
Wim LeersThis is a blocker for #2809281: Use ES6 for core JavaScript development.
Comment #77
Wim LeersComment #79
GrandmaGlassesRopeMan.eslintrc.json
rule overrides with some we'll need.npm-shrinkwrap.json
locking down dependency versions.U+1F44D
Comment #80
webchickSeems 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.
Comment #81
pfrenssenThe TWG has a meeting on February 14. This is on the list of topics to discuss.
Comment #82
mradcliffeDuplicate 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).
Comment #83
GrandmaGlassesRopeManFixes the issue in #82.
I think that marking this as a warning makes sense, especially since this this overrides the Airbnb rule.
Comment #84
tizzo CreditAttribution: tizzo commentedThe TWG has discussed this issue and are working on formulating a response. Expect an update soon.
Comment #85
GrandmaGlassesRopeMan@tizzo
Any update on this?
Comment #86
GrandmaGlassesRopeManbabel-preset-env
.last 2 versions
, andie >= 9
Comment #87
justafishThis 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)
Comment #88
finne+1
Comment #89
dmsmidt+1
Comment #90
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented+1
Comment #91
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #92
mradcliffeI 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?
Comment #93
alexpottChecking I understand this correctly - we lose linting of current core JS with this change. Right?
Comment #94
mradcliffeI 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?Comment #95
GrandmaGlassesRopeMan@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.Comment #96
pingwin4egWhy not to include
!core/**/*.es6.js
in .eslintignore? Or even simpler:!*.es6.js
?Comment #97
GrandmaGlassesRopeMan@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.Comment #98
finneOn 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.
Comment #99
alexpott@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?
Comment #100
pingwin4eg@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.
Comment #101
GrandmaGlassesRopeMan@pingwin4eg
Yes, that's correct.
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.Comment #102
GrandmaGlassesRopeMan@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.
Comment #103
GrandmaGlassesRopeManComment #104
GrandmaGlassesRopeMan@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
.Comment #105
mradcliffeAwesome, 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.
I applied the patch and got some whitespace warnings. This can probably be fixed on commit.
I updated the issue summary with this and @alexpott's question in #99.
Comment #106
GrandmaGlassesRopeMan@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.Comment #107
mradcliffeThat 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.
Comment #108
GrandmaGlassesRopeMan@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.
Comment #109
droplet CreditAttribution: droplet commentedSorry. 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?
Comment #110
GrandmaGlassesRopeManAlright.
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.I really want to avoid contrib having to change their workflow just to get stuff done.
I don't know how to handle this, but it's also not within the scope of this issue.
I think what may be left, is to review the overrides from the AirBnB standards.
Comment #111
GrandmaGlassesRopeManRemoving 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.Comment #112
droplet CreditAttribution: droplet commentedThanks @drpal.
Remaining questions:
I read a letter from 52 great Drupal Developers today. I hope we have a better transparent discussion.
Comment #113
pfrenssenThis 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.
Comment #114
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #115
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAssigning to myself per #114.
Comment #116
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThank 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: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:
Can we have inline comments for why we're making these deviations from airbnb?
Comment #117
mradcliffeI updated the issue summary.
Thank you, @effulgentsia, @tizzo and @pfrenssen.
We could do this if switched to YAML instead of JSON.
Comment #118
mradcliffeThis 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
Comment #119
GrandmaGlassesRopeManI 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 thepackage.json
and associatednode_modules
up to the root of the project to support the extension of the AirBnB standards.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.
Comment #120
mradcliffeI 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.Comment #121
GrandmaGlassesRopeManAlright. These changes should reflect the meeting nodes from DrupalCon Baltimore. Additionally this is a diff between #111 and this patch.
Comment #122
GrandmaGlassesRopeManI realized I left out the requested Yarn lock file.
Comment #123
mradcliffeI 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.
Comment #124
mradcliffeChange record is already done. Whoops. Thanks, drpal.
Moving over to the core issue to see if there are any patch fails.
Comment #125
mradcliffeRe-organized tasks.
Comment #126
droplet CreditAttribution: droplet commentedThis 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.
Comment #127
GrandmaGlassesRopeManI think this makes sense to keep.
I want to keep this for now since we already have some code that contains unused variables.
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.
Comment #128
droplet CreditAttribution: droplet commented@drpal,
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.Comment #129
GrandmaGlassesRopeMan@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.
Comment #131
mpdonadioFail was b/c https://www.drupal.org/node/2863416#comment-12062889 so it looks like we are still RTBC.
Comment #132
mradcliffeI 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.
Comment #133
alexpottI'm planning to commit this to 8.4.x tomorrow morning (UTC).
Comment #134
alexpottI 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.
Comment #135
alexpott@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'
Comment #137
xjmComment #138
mradcliffeI changed the status of the Change Record to published as this is committed.
Comment #139
droplet CreditAttribution: droplet commentedthis is outdated already! and doesn't match the issue title.
Comment #140
dawehner@droplet
Please create a new issue :)
Comment #141
GrandmaGlassesRopeMan@droplet
Created #2878620: Update AirBnB Coding standards to most recent major versions.
Comment #142
mpdonadioUpdated CR title to match the patch.
Comment #143
pfrenssenI 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:
These instructions don't seem to work out of the box:
Nothing seems to happen. To get results I need to remove the line
*.js
from.eslintignore
.This works fine. I renamed a bunch of JS files in the
misc/
folder to*.es6.js
and I get back results: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 specifyroot: true
. So this is the file I made:It is not explained how to run the command but I try it from the module folder:
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 likemodules/contrib/
. The relative path will not work in all cases.This is now my
.eslintrc.js
file:I try the command again:
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.
First off there is a typo in the filename here, it says
eslintrc.json
without the leading period.This is my
.eslintrc.json
file:I try it out after renaming the JS files to
.es6.js
:Works great!
So my results were mixed :) The legacy instructions seem to need a bit of attention.
Comment #144
alexpott@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
Comment #145
alexpott@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 .
Comment #146
alexpott@pfrenssen I've updated the CR so that all of the scenarios provided there should work.
Comment #147
pfrenssenGreat, thanks Alex!
Comment #148
droplet CreditAttribution: droplet commentedWe 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.
Comment #149
droplet CreditAttribution: droplet commentedComment #150
GrandmaGlassesRopeMan@droplet
Created #2879190: Specify a range of supported Node.js versions in package.json to specify our supported node.js versions.
Comment #152
Gábor HojtsyWhile 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.
Comment #153
mpdonadioTitle update to make it reflect what actually got committed.
Comment #154
borisson_removing tag.