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-multi-assign
Remark
- Code refactoring. Review it carefully :)
How to Review
## 1. Apply Patch
## 2. Review Code Changes
## 3. Confirm no Code Standard Errors
yarn & yarn run 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
cd core/ && yarn & yarn lint:core-js-stats
Comment | File | Size | Author |
---|---|---|---|
#28 | 2916154-13.patch | 15.47 KB | droplet |
#22 | interdiff.patch | 12.04 KB | droplet |
#22 | 2916154-22.patch | 16.99 KB | droplet |
#13 | interdiff-2916154-8-13.txt | 2.81 KB | GrandmaGlassesRopeMan |
#13 | 2916154-13.patch | 15.47 KB | GrandmaGlassesRopeMan |
Comments
Comment #2
GrandmaGlassesRopeManComment #3
dawehnerI know its not needed but afaik we use semicolons in all our JS.
In the other case we would have used
this.name = this.pristine
In this case for example we use model just once. Could we not just get rid of it or do you think this would be out of scope here? At least for me a changed line feels better than a new line.
This is another one of those cases.
Comment #4
GrandmaGlassesRopeMan- #3.1 - Nice catch.
- #3.2 - Yes. Since these are declared right at the top of the function, they are both just assigned the state prop.
- #3.3/4 - There was only one other location when the variable,
model
, was used. If the tests pass then it's probably alright.Comment #5
dawehnerThank you matt!
Comment #6
alexpottIn reviewing the patch I was wondering why this is important... https://github.com/airbnb/javascript#variables--no-chain-assignment
After applying the patch I get an error:
Comment #7
droplet CreditAttribution: droplet commentedIn JS, sometimes, it's very easy to overlook. To check out the example code below:
http://jsbin.com/bayajepule/1/edit?js,console
Comment #8
GrandmaGlassesRopeMan@alexpott Nice catch. Thanks.
Comment #9
alexpott@droplet thanks - I didn't complete my thought on #6 - the link https://github.com/airbnb/javascript#variables--no-chain-assignment also explains very nicely why this is important.
Comment #10
dawehnerHa, yeah they are not just braindead changes.
Comment #11
xjmIn this one, couldn't we just have one line that does
this.scrollY = scrollY
after the conditional? Since it's in both code paths identically.Same here for
this.$textElement
.Comment #12
dawehnerI'd love to have @xjm's eagle eyes on code reviews.
Comment #13
GrandmaGlassesRopeMan#11.1 - 🤘
#11.2 - 👍
Comment #14
dawehner+1 for making this sane code overall
Comment #15
alexpottCrediting @xjm for the review in #11.
Comment #16
droplet CreditAttribution: droplet commentedNeeds docs update on this changes I think. And reverse the order seems more sense? So `states` is caching `Drupal.states` locally.
same as above, reverse the order?
- toolbar.es6.js
- plainTextEditor.es6.js
has same issues.
Why not use `this.windowHeight` directly? ( I will file another issue for the OR condition, I think this is outdated! It's for IE6~8 )
Comment #19
alexpottI crossposted a commit with @droplet - I reverted because of that.
Re the last part of @droplet's review in #16 I pondered this too but since removing outdated code paths for ie6-8 is not in scope I didn't mention it. I also didn't ask for a follow-up because doing that for all our existing JS is likely to an enormous issue and one that should have very careful scoping in order to be successful.
Comment #20
dawehnerI totally think we should use this.... only in all those instances. What about going through the code in a follow up and clean all those places up?
Comment #21
droplet CreditAttribution: droplet commentedI think no. 2 reasons:
1. this is a simple change
2. we should have 100% confidence to make this change. (I would wonder why not? Is that one of http://jsbin.com/bayajepule/1/edit?js,console issues)
One patch to final, no new introduced issue :)
Comment #22
droplet CreditAttribution: droplet commentedexplains why I did it:
with this change, we know it will be Object, not string assign. (Needs follow-up to fix docs so that has a better editor hints)
no helps for performance
I don't think we need a doc to explain everything. Code speaks itself already.
only have to explain unexpected behaviors (e.g. may fall into #7 examples)
Comment #23
dawehnerAt least for me having this const up here and then merged as default on the other end of the file is a bit confusing. Maybe that's just me
Right, but this change is also simply not needed. We talked about the change and the core committer will stumble upon it as well, I'll promise you :) Still I agree with you that the comment is not needed actually.
Comment #24
GrandmaGlassesRopeManWhy can't we just reference
Drupal.states
throughout?Again here, why are we not just referencing
this.$formContainer
?this.$textElement
?Drupal.toolbar.models.toolbarModel
?Comment #25
dawehnerI agree with @drpal that not having an additional variable is the ideal future. I personally think smaller steps are better, but I refer to the JS maintainers, @droplet and @drpal to make this call.
Comment #26
GrandmaGlassesRopeMan@dawehner ➕1️⃣
Smaller steps makes it easier to review. There's always the option for a followup to cleanup some additional issues that are not specifically related to the codestyle fixes in the issue.
Comment #27
droplet CreditAttribution: droplet commentedOK. I just give up :) Not worth to make a argue here for this little thing.
And the reason I trying to do a little more because I see @xjm comments and you're agreed :p
Comment #28
droplet CreditAttribution: droplet commentedI re-upload @drpal' #13.
Thanks All!
Comment #29
droplet CreditAttribution: droplet commentedSorry, I forgot about it. Without my changes, it still needs a follow-up for the docs I pointed in #22. With #13 changes, the docs don't correct.
Comment #30
xjmThanks everyone -- I agree with keeping the scope of this constrained to things that can be reviewed in the immediate context of the patch. That way we can review and confirm the changes are correct without having to understand the whole file.
Removing totally unnecessary local variables is also useful though, as is removing dead code or unnecessary docs. I agree that those things can happen in a followup issue. Shall we file issues for those things and reference them here?
Comment #32
xjmMeanwhile, committed to 8.5.x. Thanks!
Comment #33
droplet CreditAttribution: droplet commentedThanks.
Comment #34
dawehner1️⃣ #2918471: Remove local variables and replace them by this. or Drupal.states.