Follow-up to #2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib

Problem/Motivation

Our versions of the AirBnB JavaScript coding standards are out of date and should be updated.

Proposed resolution

Update versions of our coding standards dependencies.

CommentFileSizeAuthor
#12 2878620-12.patch56.96 KBGrandmaGlassesRopeMan
#12 interdiff-2878620-9-12.txt10.5 KBGrandmaGlassesRopeMan
#9 upgrade-eslint-packages.patch52 KBdroplet
#7 2878620-7.patch14.32 KBGrandmaGlassesRopeMan
coding-standards-version-bumps.patch6.56 KBGrandmaGlassesRopeMan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drpal created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Thank you @drpal!

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Issue tags: +Needs change record updates 
+++ b/core/package.json
@@ -14,10 +14,10 @@
-    "eslint-plugin-jsx-a11y": "4.0.0",
-    "eslint-plugin-react": "6.10.3",
+    "eslint-plugin-jsx-a11y": "5.0.1",
+    "eslint-plugin-react": "7.0.1",

IS mentions AirBnB JavaScript coding standards, but these two aren't part of those, correct? Maybe this just needs an IS update to "bump all of things for 8.4 since the other issue sat for a while".

I think a larger question is what the policy will be for keeping these up to date. Keeping in line with other things would be to bump near the beginning of the dev cycle and then keep stable so people don't have a moving target.

Probably also want to update the CR to match the final version of this patch so it will be accurate for 8.4.0.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

@mpdonadio

Both eslint-plugin-jsx-a11y and eslint-plugin-react are dependencies so they need to be updated as well. I'm sure we'll do this again as we get closer to 8.4 so I'll update the change record once this goes in along with a final update before 8.4.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Maybe a really naive question: What is the reason we don't use something like ^5.0.0 in our package.json files? We still have the yarn lock file, so its actually locked. In the composer.json we actually run with them.

droplet’s picture

droplet CreditAttribution: droplet commented

@dawehner,

We can do it now. It was no lock file & Yarn. But with lock file now, do it or not seems no much diff :)

+++ b/core/package.json
@@ -14,10 +14,10 @@
+    "eslint-config-airbnb": "15.0.0",

this is updated again with docs & version fixes. We can wait for one more week to see if there's any upstream quick fix on this new release.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
FileSize
2878620-7.patch14.32 KB

- Updating eslint, eslint-config-airbnb, eslint-plugin-import, eslint-plugin-jsx-a11y, and eslint-plugin-react to their most recent versions.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
upgrade-eslint-packages.patch52 KB
martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs review » Reviewed & tested by the community

When I typed

yarn outdated... just now to set my expectations

I see all the version updates I expect in the package.json file.

so + 1 from me.

BTW we are now updating

eslint-config-airbnb 14.1.0 -> 15.1.0

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Reviewed & tested by the community » Needs work

This causes a warning warning "eslint-config-airbnb@15.1.0" has incorrect peer dependency "eslint-plugin-jsx-a11y@^5.1.1". which we'll have to fix.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-2878620-9-12.txt10.5 KB
2878620-12.patch56.96 KB

- Fixed dependencies.

Additionally, AirBnB has some strange behavior with its dependencies.

I've done, npm info eslint-config-airbnb@latest peerDependencies --json,

Which returns, 

{
  "eslint": "^3.19.0 || ^4.3.0",
  "eslint-plugin-jsx-a11y": "^5.1.1",
  "eslint-plugin-import": "^2.7.0",
  "eslint-plugin-react": "^7.1.0"
}

And then installed them, which you can do automatically, 

(
  export PKG=eslint-config-airbnb;
  npm info "$PKG@latest" peerDependencies --json | command sed 's/[\{\},]//g ; s/: /@/g' | xargs yarn add --dev "$PKG@latest"
)

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Is this issue still needed once we switch to prettier?

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs review » Closed (outdated)

This is probably out of date anyways. We can close this in favor of, #2978964: Use Prettier for formatting core JavaScript