Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
You can check the current state of coding standards compliance using eslint:
yarn install
yarn run lint:core-js-passing
They are powered via the core/.eslintrc.passing.json configuration file
Background
We adapted the airbnb coding standard (#2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib), but we are not fully compliant to it yet.
Proposed solution
We split up the work of fixing the coding standards into small chunks. These chunks should be one eslintrc rule, so the individual patches are easy to review, just like we are doing it in #2571965: [meta] Fix PHP coding standards in core.
Approach
- Pick a rule, like “no-mutable-exports” and ensure they are not longer excluded from the .eslintrc.json file, see #2912961: Step 0 JS codestyle: Exclude non passing eslint rules
- Copy the issue template, linked below, and put in your picked rule
- Link this issue to it using the parent field in the issue
- Fix all of the instances of this rule, but NO others
- Verify by using
yarn run lint:core-js | grep no-mutable-exports | wc -l
- Review others people’s issues
Comments
Comment #2
dawehnerComment #3
dawehnerComment #4
nod_Comment #5
xjmI'm on the fence as to whether we should want to backport these standard fixes to 8.4.x. #2912961: Step 0 JS codestyle: Exclude non passing eslint rules is only available for 8.5.x and backporting it would not be semver-compliant, but we could still backport the cleanups themselves since if they're working as intended they shouldn't be making functional changes.
On the one hand, if we don't, the branches will diverge and make cherry-picking actual bugfixes harder. On the other hand, if we do, we could inadvertently break something we don't have test coverage for, especially for issues more complicated than this one. I'll ask other committers for an opinion.
Comment #6
dawehnerPersonally I think the CS patches we had so far have been so minimal that i think the risk of diversion is quite small, but maybe I'm simply underestimating the problem.
Comment #7
xjmThe scope of what we've seen so far is small, yeah. OTOH there's thousands to go before we sleep, including patches like #2915784: 1/3 JS codestyle: camelcase which is big and also contains variable renaming -- which is a classic "gotcha" for introducing bugs with cleanups. So they could be risky and we might have the branches diverge. And that one is on the edge for a cleanup we'd commit as a scheduled RC target. Not sure.
We can probably continue committing them to 8.5.x for now and backport them if we have a concrete need or find that other backports are becoming tediously divergent from their parent patches.
Comment #8
dawehnerGood point, let's not have some premature optimization going on.
Comment #10
elachlan CreditAttribution: elachlan commentedComment #11
elachlan CreditAttribution: elachlan commentedComment #21
quietone CreditAttribution: quietone at PreviousNext commented