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.
Style error information
https://eslint.org/docs/rules/no-use-before-define
Remark
Code Improve. Less harmful.
How to Review
## 1. Apply Patch
## 2. Review Code Changes
## 3. Confirm no Code Standard Errors
yarn & yarn lint:core-js-passing
## 4.1 If `NO` errors, mark the issue as `Reviewed & tested by the community` (Don't be shy, we're all friendly)
## 4.2 If `HAS` errors, fix it and upload a new patch (Just do it and you can!!!)
Background
- #2912962: Step 1 JS codestyle: [meta] Fix JS coding standards in core
- 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.
More Information
- Using ES6 in Core
https://www.drupal.org/node/2815083
- To find JS code standard errors stats
yarn & yarn lint:core-js-stats
Comment | File | Size | Author |
---|---|---|---|
#14 | 2946603-14.patch | 156.67 KB | alexpott |
#14 | 8-14-interdiff.txt | 2.84 KB | alexpott |
#11 | 2946603-11.patch | 327.75 KB | alexpott |
#11 | 8-11-interdiff.txt | 173.93 KB | alexpott |
#8 | interdiff-2946603-5-8.txt | 437 bytes | GrandmaGlassesRopeMan |
Comments
Comment #2
GrandmaGlassesRopeMan- This patch is quite large, 154.93 KB
Comment #3
borisson_I ran
yarn lint:core-js-passing
before and after applying the patch and got the same result.Comment #4
dawehnerI went through the patch in phpstorm and tried to imagine possible problems (Altering "global" state). This patch improves the situation in these cases!
I'm curious, what is the reason behind that?
I just checked this one ... we are calling A in function B and B in function A.
This is much nicer here. We now modify a value in a function which is used afterwards. 👍
Should we move
$summaries
after theif ($!$details.length)
?I know it shouldn't be an issue, but i think checking the result of$details
first, still makes sense.Comment #5
GrandmaGlassesRopeMan@dawehner -
#4.1 - This one is a bit difficult to see,
function shutdown() { //... }
is actually part offunction closeDialog(action, form) { //... }
which is used in creation of the actualdialog
.#4.2 - Yes, without some major rewriting, it wasn't worth it.
#4.3 - 👍
#4.4(?) - Good point. I guess it could potentially throw an error.
Comment #6
dawehnerThank you @drpal!
Comment #7
alexpottThis doesn't really make sense here anymore. It was a kind of divider of the file. I think we should just remove it.
This one is really not easy to review.
Comment #8
GrandmaGlassesRopeMan@alexpott
#7.1 - Yep.
#7.2 - Yeah, sorry. The changes to
quickedit.es6.js
are moving a very large chunk of the file to the bottom from the top.#7.3 - I don't really know. Some of the instances are minor, but having a followup for someone to investigate might be nice.
Comment #9
dawehnerThank you @drpal for fixing it!
Comment #10
alexpottSeems as though a recent change has introduced a new problem.
Comment #11
alexpottMoved the method.
Comment #12
alexpottCreated followup - #2949816: Fix remaining no-use-before-define eslint errors
Comment #13
dawehnerDoes someone mind to not add the yarn-error log?
Comment #14
alexpottWhoopsie. Interdiff back to #8. Sorry for the noise.
Comment #15
dawehnerThank you alex
Comment #16
alexpottCommitted 7d2f3a3 and pushed to 8.6.x. Thanks!