Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Child issue of #2917234: [2/2] JS codestyle: no-restricted-syntax which has the non easy to review changes removed.
Comment | File | Size | Author |
---|---|---|---|
#70 | interdiff-2925064-53-69.txt | 1.32 KB | GrandmaGlassesRopeMan |
#70 | 2925064-69.patch | 89.88 KB | GrandmaGlassesRopeMan |
#53 | interdiff-2925064-50-53.txt | 902 bytes | GrandmaGlassesRopeMan |
#53 | 2925064-53.patch | 89.45 KB | GrandmaGlassesRopeMan |
#50 | 2925064-50.patch | 89.52 KB | GrandmaGlassesRopeMan |
Comments
Comment #2
dawehnerComment #3
dawehnerComment #4
dawehnerComment #5
dawehnerComment #6
dawehnerComment #7
GrandmaGlassesRopeMan- fixes a few missed cases
- applies
eslint-disable-next-line
oreslint-disable-line
where we usebreak;
orcontinue;
Comment #8
xjmI'd pull this out since it doesn't match the rest of the totally simple changes. The smaller we can make this first patch, the better.
Edit: Removed 2 and 3 since they were in the transpiled ES5 code, sorry!
Comment #9
xjmI'd suggest making a first patch that is only the changes that are super simple and limited to for + hasOwnProperty => forEach without the need to remove/reassign/etc. and without more complicated looping logic. That way the big patch is all things that can be reviewed with a word diff of exactly the same thing over and over, and the smaller patch (or patches) are the ones where the reviewer has to actually think rather than just check that the right name is moved to the right place.
Comment #11
xjmIdeally my word diff will contain only hunks that look almost exactly like this:
That's:
(The regex could probably be refined but you get the idea.)
Comment #12
xjmI'm dumb; this is a transpiled file. Well, now I know how it gets transpiled. :)
Edit: This might actually be an issue where it's worth providing a patch for reviewers that's not intended for commit, containing only the ES6 code, so there's just less to read that way as well.
Comment #13
GrandmaGlassesRopeManI'll post an es6 only changes patch soon.
Comment #14
dawehnerI splitted up the issues in the way how I would have liked to see it as a reviewer, maybe there was some misunderstanding
Personally I review these patches in my IDE, which makes it easy to see scope. Just looking at the git diff, at least for me, never gives enough context to really judge changes.
I'm curious, is this different to any other issue with JS changes involved?
Comment #15
GrandmaGlassesRopeMan- Fixes a minor error with returns in
tabledrag.es6.js
Comment #16
GrandmaGlassesRopeManComment #17
xjmYes, I think it is. There's research that shows that code review quality declines significantly for changes much larger than 100 lines or so, and that reviews of changes that are more than 200 lines will miss more than half the bugs in them. (There's another study I'm thinking of but it wasn't in my bookmarks).
For a small bugfix, context-switching between the ES6 and ES5 isn't too bad, and I do also want to review the ES5 to double-check that there isn't a regression. However, for a patch that has 40+ different files in it, context switching back and forth between the two 20+ times is hard and more likely to lead to errors.
If 50K a patch contains only the exact sort of change over and over, one that's identical in a word diff, I don't have to parse the code in my head and I can get through the patch within well under an hour. I can just make sure that a for loop is replaced with
Object.keys.forEach(thing)
and thatthing.hasOwnProperty
is removed. It becomes just pattern-matching. I don't have to pay attention to the filename or the surrounding lines or what the code actually does.The fact that @drpal was able to introduce a bug in tabledrag here indicates to me that that one should go in a separate issue. :)
Comment #18
dawehner@xjm
Thank you for the detailed answer. I agree, the bigger a patch is, the less you think about particular changes.
Well, this the problem, its not that simple. By switching from a for loop to a function you change the semantic of things.
Let's look at this example:
Before
After
It turns out, having a minimal diff would not detect this problem. What I try to stress out here is, due to the nature of the change, a minimal diff, which you just pattern match by, doesn't actually allow you to actually see, whether the change is the right one. You need the full context.
This is special in this case compared to most other CS issues we usually have.
Given the paper you linked, an alternative approach would be: Limit the set of files we want to change and split it up in maybe 4 packages or so?
Comment #19
xjmWe could split off things like tabledrag and states that are big and complicated? Things where the looping variable wasn't already initialized in the
for
itself? I guess if the transpiled code is different from the ES6 code, that's also a signal that something more complicated is going on.Comment #20
droplet CreditAttribution: droplet commentedthat study told you to review 200/400 loc per time, not force contributors to split their work into 200~400 loc or per contributions. With less relative context you could make more mistake also. Trying not to doing matching game on code review.
No matter how many threads you split, reviewers still able to review as many as they wanted within a short period. So that, the only thing you could do is either to trust the reviewers or to control your own flow to review that again.
Or might be asking for something like this:
https://www.drupal.org/project/drupal/issues/2911171
with Git `format-patch` you can try to guess or understand what the contributor did. And stopping to review at the right time when you feel tired.
Comment #21
dawehnerShould we do those maybe first?
Comment #22
justafishJust looked through the patch (looks good!) but I'm not really sure how we can split this up to make it easier, other than giving you a separate patch per file. However, maybe this is helpful:
- patch applied as 2 commits, with ES6 and compiled changes separated: https://github.com/justafish/drupal/compare/8.5.x...2925064-js-codestyle
- which then gives you just a diff of the ES6 changes: https://github.com/justafish/drupal/commit/b75821882fba5ab75e4a24a652eb6...
Comment #23
xjmThe idea is that there are parts of this that I can commit, but others that I probably wouldn't touch and so they could get blocked for awhile on frontend framework review. And if it's 100K it's going to get blocked for awhile no matter what because all committers will avoid slogging through it especially for a coding standards cleanup.
We have the scope guidelines for a reason; they smooth the way for patches to land based on what reviewers can and can't accomplish in a timely fashion. :) It doesn't need to be one patch per file (edit: it shouldn't be in fact; that has the opposite problem of significant overhead for everyone) but I would suggest dedicated patches for APIs that have a bigger scope.
Comment #24
xjmOh, that's https://www.drupal.org/core/scope for those that might not be familiar (@drpal wasn't before last week).
Comment #25
justafishIt looks to me like each file is it's own area/scope, so I'm still not sure how to split this patch up differently than per file. However, the diff without the compiled/auto-generated code is about half the size you mentioned - maybe you could have a look through that ( here: https://github.com/justafish/drupal/commit/b75821882fba5ab75e4a24a652eb6... ) and mark the parts with a comment that you aren't comfortable with reviewing, and we can split it off on that basis?
Comment #26
dawehnerHere is an attempt to minimise the changes needed.
Still, some of these changes look simple, but can you prove that they are fine? I don't think you can, because you need to look at more context.
Just because these changes now look simple, doesn't mean that they are exactly the ones needed.
For me at least we should trust people who know JS that they know what they’re trying to achieve, but this is just my 2 cents.
Comment #28
dawehnerThe interdiff is still the same, the patch was completely wrong.
Comment #29
webchickTrying to balance concerns about scoping with burden on reviewers by additional issues, I sat with @justafish, @drpal, and @dawehner for like 2 hours over hangouts and they stepped me through the entire diff at https://github.com/justafish/drupal/commit/b75821882fba5ab75e4a24a652eb6... line-by-line (side-note: GitHub's code review tools are pretty nifty ;)), bless their hearts. ;)
For the most part, this patch is trying to remove all instances of:
(This approach is because a simple for FOO in BAR will get you 100s of other properties auto-added by JavaScript that have nothing to do with your object definition.)
Usually, these are replaced with (+/- some indentation):
Which is also much easier to read, so yay!
There are a couple of places where these replacements are not straight-forward, and these are all areas where the loop has to be broken early for some reason. (e.g. we conditionally want to return true or false based on some condition.) This is because .forEach() doesn't have a way to break out early.
The only problem I really see with the patch is that when the above condition happens, there are two different approaches taken, depending on the hunk:
1) Append (or prepend)
// eslint-disable-line no-restricted-syntax
to the for ... in loop so it gets skipped in checks, for example:2) Re-writing the for ... in loop as a "traditional" for loop, as in:
I'm happy with either approach; we should just pick one that's consistent throughout. If we go with the first approach, I would extra especially love to see a brief comment ahead of the disable comment that explains why we're doing that so the next person doesn't need to ask the same questions. :)
One other thing that came up was this hunk fell into the "one of these things is not like the other" bucket:
I guess that's since been fixed and will be in the next patch.
And finally, the hunk where we change
_calculateAutoAllowedTags()
to change the scope of the variousfeatureName
,featureRule
, etc. That seemed out of scope for a patch that's generally otherwise trying to do as little "side quests" as possible to modernize the code. But this was explicitly added by @droplet over in #2917234-18: [2/2] JS codestyle: no-restricted-syntax to help prevent no-shadow issues.Speaking of @droplet, he also notes that simply reading this patch is dangerous because it doesn't reveal all of the surrounding context, which is a fair concern. To address that concern, I'm going to manually test various areas that this patch touches to look for errors/regressions (will post back when I'm done; using something like https://www.drupal.org/node/1777342 as a starting point). I think between the two, and given we still have another 4 months before 8.5 is released, this should be sufficient for getting this patch in, so @lauriii and others with front-end expertise can use their energy for patches that need more architectural review.
Comment #30
webchickOk, in terms of manual testing, threw the patch at #28 on simplytest.me, and verified...
- CKEditor (text formatting, image uploading, captions, toolbar customization)
- AJAX file upload + removal
- Quick edit (formatting changes to body, title change)
- Changing Color module settings + preview updated
- Table drag on menus (both up/down and indent/outdent)
- Collapsible fieldsets, vertical tabs (user account settings admin page)
- Settings tray (enter edit mode, expand sidebar, save)
- Toolbar (expand/collapse, side/top)
- Views UI (dropbuttons, state system, AJAX preview)
While that's not necessarily every area touched by the patch, it does hit on most of them and I discovered no issues. So as long as the next patch fares the same, I think we're good to go here.
Comment #31
GrandmaGlassesRopeManAlright, i think this gets everything? If you're into side-by-side views, there's one here, https://github.com/mattgrill/drupal/compare/8.5.x...mattgrill:2925064
Comment #32
dawehnerJust an example that just the patch file doesn't cut it. In this case we add new shadowing, but its fine, as in the function ends afterwards.
This should be still using
eslint-disable-next-line
.Comment #33
GrandmaGlassesRopeMan@dawehner 👏
#32.2 - fixed
Comment #34
dawehnerThank you @drpal!
Comment #35
webchickOk, re-ran through my "test script" at #30 with the latest patch, and all looks good. Code changes also are nice and consistent now, thank you!
Committed and pushed to 8.5.x. Woohoo!
Comment #37
droplet CreditAttribution: droplet commented#2927407: Follow-up: remove unnecessary returns in ajax.js
Comment #38
droplet CreditAttribution: droplet commented#2927413: Remove extra var assignment in Drupal.stringReplace()
Comment #39
droplet CreditAttribution: droplet commentedseems like to revert this patch is a better action, I felt a bit tried to match the patched version & split patch one-by-one to make a fix.
It's OK:
It's broken:
Object.keys(undefined).forEach()
There's few places like this:
this.tableSettings = tableSettings;
In a better structured JS, we able to do this (even no ||):
this.tableSettings = tableSettings || {};
but in Drupal, I suggested adding extra {} to
Object.keys
for now (before a full-suite testing)Thanks
(The patch I uploaded is a ongoing-patch. Don't review and Don't commit it.)
Comment #40
xjm@droplet, can you confirm that you think the previous commit needs to be reverted, because it introduced broken code for
Object.keys(undefined).forEach()
? If it broke something I agree we should revert (and then split it up so reviewers have a better chance of finding defects in the review).Otherwise if we don't want to revert the previous commit, we should mark this issue back to fixed and create a followup for your additional fix.
Let me know which I should do (or maybe @dawehner can help confirm). Thanks!
Comment #41
droplet CreditAttribution: droplet commentedIt potentially breaks the careless contrib code IMO.
For the CORE, I don't know. I have to review them one-by-one to figure out.
If we able to revert it, then we can simply add it to everywhere rather than searching them from our patched code again. (Also hear @dawehner comments first)
Comment #42
dawehnerWhile I totally think we should fix these annoying bits (did I told anyone that I like typesystems :P), I don't think it is a likely issue
a) Drupal just calls these APIs when it actually has values
b) Why would there be other code out there calling for example tabledrag without parameters.
Still, I think reverting it cheap, let's do it.
Comment #43
droplet CreditAttribution: droplet commentedhaha, that helps but not at all. With current JSDoc, @ts-check helps a bit if you're a TS fans. (or automatic with VSCode. But yeah, still far from a fully typed system)
Not at all. Some values come from HTML markup and modified via Ajax after the initial. The Drupal.behaviors system is stupid, always looping overall.
We could close as it and counts contrib code as buggy, not CORE.
Some code, for example tabledrag, we still need a second checking. We may modify our code in the circuit, e.g.:
In Drupal, I think many of times we don't have the SAME vision. It ends up many bits I pointed out in #39. For example, the below issue, if it's a solo project, I will close it directly. (I will not fulfill everyone's needs and change the designed way/code pattern. LOL)
https://www.drupal.org/project/drupal/issues/2551373#comment-11931356
Comment #44
dawehnerCan you explain that more? I simply don't understand it.
I think we are all simply enthusiastic, so let's try to be productive :)
Comment #45
droplet CreditAttribution: droplet commentedTake this as example, (no typed docs)
const trigger = states.Trigger.states[this.state];
You able to assign `null` to `trigger`. I cannot tell it's wrong before this patch. (and I quite sure we will fix these problems when someone makes a complaint)
"be productive" is nothing wrong. The problem is the code becomes unmaintainable over time and boated. It's ridiculous because my #39 comment is doing what I dislike. But sometimes, we still have to take care of the legacy codebase. :s
Revert this patch or not both make sense to me. Just different angle to see things.
Comment #47
xjmSo to revert this, I had to revert both of the camelcase patches, because there were merge conflicts. :(
Let's postpone this issue on those three, since they are comparatively straightforward whereas this one involves changes to actual code.
I encourage the discussion to continue though. I agree with @droplet that we should do what we can to minimize disruptions to contrib code (even contrib code that isn't careful or doesn't follow best practices) as we work through these cleanups.
Comment #48
webchickHey, @droplet! Thanks for the follow-ups. Is there any way for you to join #javascript on Drupal Slack? (See https://www.drupal.org/slack for instructions.) Real-time communication might make these and other patches go more smoothly. :)
Comment #49
webchickOk, camelCase is in (I hope); this one will need a re-roll.
Comment #50
GrandmaGlassesRopeMan- Reroll.
- Handle feedback about making
Object.keys()
safer from #39. Additionally, not all instances ofObject.keys()
needed an additional|| {}
check due to previously present validation.Comment #51
justafishcouldn't this be just:
keys = Object.keys(args || {})
?Comment #52
dawehnerYou are totally right, this code can be dramatically simplified. Last time we discussed that the argument was: Let's try to minimize changes as much as possible.
Comment #53
GrandmaGlassesRopeMan- #51.1 - fixed. Nice catch here. I think it's totally reasonable to make this change. 👏🎉
Comment #54
justafish👍
Comment #55
droplet CreditAttribution: droplet commentedIt's miracle, the same thing with diff feedback :p
https://www.drupal.org/project/drupal/issues/2927413
Comment #57
GrandmaGlassesRopeMan🤷♀️
Comment #58
GrandmaGlassesRopeManRandom ci failures!
Comment #59
dawehner@droplet
You are absolutely right that the original patch now contains parts of this patch, still your patch has the merged sort bit as well.
Comment #61
GrandmaGlassesRopeManLooks like a random failure from ci.
Comment #63
GrandmaGlassesRopeMan🤷♀️
Comment #65
GrandmaGlassesRopeManComment #67
GrandmaGlassesRopeManI'll just keep doing this...
Comment #68
webchickOk, take two!
Committed and pushed to 8.5.x. Thanks!
Comment #69
webchickHa, nevermind. It doesn't like:
Comment #70
GrandmaGlassesRopeMan- fixes a minor coding standards issue in
states.es6.js
Comment #71
dawehnerI'm constantly surprised by what babel is capable of.
Comment #72
webchickThird time's a charm..? :P
Committed and pushed to 8.5.x. Thanks! (??!?)
Comment #76
larowlanPosthumously adding credit for @martin107 who worked on a similar fix in #2909180: Remove potentially unperformant lint errors in ajax.es6.js for code in ajax.es6.js
#BugSmash initiative just closed that as a duplicate.