Child issue of #2917234: [2/2] JS codestyle: no-restricted-syntax which has the non easy to review changes removed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Active » Needs review
dawehner’s picture

Issue tags: +JavaScript
dawehner’s picture

GrandmaGlassesRopeMan’s picture

- fixes a few missed cases
- applies eslint-disable-next-line or eslint-disable-line where we use break; or continue;

xjm’s picture

  1. +++ b/core/misc/states.es6.js
    @@ -267,14 +262,15 @@
    -        for (const n in constraints) {
    -          if (constraints.hasOwnProperty(n)) {
    -            result = ternary(result, this.checkConstraints(constraints[n], selector, n));
    -            // False and anything else will evaluate to false, so return when
    -            // any false condition is found.
    -            if (result === false) {
    -              return false;
    -            }
    +        const constraintsIds = Object.keys(constraints);
    +        const length = constraintsIds.length;
    +        for (let i = 0; i < length; i++) {
    +          const n = constraintsIds[i];
    +          result = ternary(result, this.checkConstraints(constraints[n], selector, n));
    +          // False and anything else will evaluate to false, so return when
    +          // any false condition is found.
    +          if (result === false) {
    +            return false;
    

    I'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!

xjm’s picture

I'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.

Status: Needs review » Needs work

The last submitted patch, 7: 2925064-6.patch, failed testing. View results

xjm’s picture

Issue summary: View changes
FileSize
83.04 KB

Ideally my word diff will contain only hunks that look almost exactly like this:

That's:

git diff --staged --color-words="[^[:space:]\(\)\{\}]+|[\(\)\{\},]+"

(The regex could probably be refined but you get the idea.)

xjm’s picture

+++ b/core/misc/ajax.js
@@ -395,6 +391,8 @@ function _toConsumableArray(arr) { if (Array.isArray(arr)) { for (var i = 0, arr
+    var _this = this;
+

I'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.

GrandmaGlassesRopeMan’s picture

I'll post an es6 only changes patch soon.

dawehner’s picture

I 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.

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.

I'm curious, is this different to any other issue with JS changes involved?

GrandmaGlassesRopeMan’s picture

- Fixes a minor error with returns in tabledrag.es6.js

GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
xjm’s picture

I'm curious, is this different to any other issue with JS changes involved?

Yes, 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 that thing.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. :)

dawehner’s picture

@xjm
Thank you for the detailed answer. I agree, the bigger a patch is, the less you think about particular changes.

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 that thing.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.

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

let name = '';

for (name in names) {
}

console.log(name);

After

let name = '';

names.forEach((name) => ...)

console.log(name);

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?

xjm’s picture

We 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.

droplet’s picture

that 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.

dawehner’s picture

We could split off things like tabledrag and states that are big and complicated?

Should we do those maybe first?

justafish’s picture

Just 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...

xjm’s picture

The 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.

xjm’s picture

Oh, that's https://www.drupal.org/core/scope for those that might not be familiar (@drpal wasn't before last week).

justafish’s picture

It 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?

dawehner’s picture

Here 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.

Status: Needs review » Needs work

The last submitted patch, 26: 2925064-24.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
89.13 KB

The interdiff is still the same, the patch was completely wrong.

webchick’s picture

Status: Needs review » Needs work

Trying 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:

for (FOO in BAR) {
  if (BAR.hasOwnProperty(FOO)) {
    ...
  }
}

(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):

Object.keys(BAR).forEach((FOO) => {
  ...
});

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:

for (const i in dependeeStates) { // eslint-disable-line no-restricted-syntax

2) Re-writing the for ... in loop as a "traditional" for loop, as in:

+        const constraintsIds = Object.keys(constraints);
+        const length = constraintsIds.length;
+        for (let i = 0; i < length; i++) {
+          const n = constraintsIds[i];
...

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:

 +      const filterConfiguration = Drupal.filterConfiguration;
 +      const filterStatuses = filterConfiguration.statuses;
 +      Object.keys(filterStatuses).forEach((filterID) => {
 +        const filterStatusId = filterStatuses[filterID];

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 various featureName, 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.

webchick’s picture

Ok, 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.

GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
FileSize
89.11 KB
8 KB

Alright, 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

dawehner’s picture

  1. +++ b/core/misc/states.es6.js
    @@ -38,15 +38,13 @@
           const il = $states.length;
           for (let i = 0; i < il; i++) {
             config = JSON.parse($states[i].getAttribute('data-drupal-states'));
    -        for (state in config) {
    -          if (config.hasOwnProperty(state)) {
    -            new states.Dependent({
    -              element: $($states[i]),
    -              state: states.State.sanitize(state),
    -              constraints: config[state],
    -            });
    -          }
    -        }
    +        Object.keys(config).forEach((state) => {
    +          new states.Dependent({
    

    Just 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.

  2. +++ b/core/misc/tabledrag.es6.js
    @@ -264,30 +258,28 @@
    +      for (const d in this.tableSettings[group]) { // eslint-disable-line no-restricted-syntax
    

    This should be still using eslint-disable-next-line.

GrandmaGlassesRopeMan’s picture

@dawehner 👏

#32.2 - fixed

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @drpal!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, 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!

  • webchick committed b0a4d7e on 8.5.x
    Issue #2925064 by drpal, dawehner, xjm, justafish, droplet: [1/2] JS...
droplet’s picture

droplet’s picture

Status: Fixed » Needs review
FileSize
3.69 KB

seems 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:

for (const i in undefined){}

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.)

xjm’s picture

@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!

droplet’s picture

It 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)

dawehner’s picture

While 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.

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)

Still, I think reverting it cheap, let's do it.

droplet’s picture

While 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

haha, 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)

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.

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.:

hidden = this.tableSettings[group][d].hidden;

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

dawehner’s picture

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.

Can you explain that more? I simply don't understand it.

. (I will not fulfill everyone's needs and change the designed way/code pattern. LOL)

I think we are all simply enthusiastic, so let's try to be productive :)

droplet’s picture

Take 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)

I think we are all simply enthusiastic, so let's try to be productive :)

"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.

  • xjm committed dcfee31 on 8.5.x
    Revert "Issue #2925064 by drpal, dawehner, xjm, justafish, droplet: [1/2...
xjm’s picture

Status: Needs review » Postponed

So 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.

webchick’s picture

Hey, @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. :)

webchick’s picture

Status: Postponed » Needs work

Ok, camelCase is in (I hope); this one will need a re-roll.

GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
FileSize
89.52 KB

- Reroll.
- Handle feedback about making Object.keys() safer from #39. Additionally, not all instances of Object.keys() needed an additional || {} check due to previously present validation.

justafish’s picture

+++ b/core/misc/drupal.es6.js
@@ -317,11 +315,7 @@ window.Drupal = { behaviors: {}, locale: {} };
+      Object.keys(args || {}).forEach(key => keys.push(key));

couldn't this be just:
keys = Object.keys(args || {})?

dawehner’s picture

keys = Object.keys(args || {})?

You 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.

GrandmaGlassesRopeMan’s picture

- #51.1 - fixed. Nice catch here. I think it's totally reasonable to make this change. 👏🎉

justafish’s picture

Status: Needs review » Reviewed & tested by the community

👍

droplet’s picture

It's miracle, the same thing with diff feedback :p

https://www.drupal.org/project/drupal/issues/2927413

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 2925064-53.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

🤷‍♀️

GrandmaGlassesRopeMan’s picture

Status: Needs work » Reviewed & tested by the community

Random ci failures!

dawehner’s picture

@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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 2925064-53.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

Status: Needs work » Reviewed & tested by the community

Looks like a random failure from ci.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 2925064-53.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

Status: Needs work » Reviewed & tested by the community

🤷‍♀️

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 2925064-53.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 2925064-53.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

Status: Needs work » Reviewed & tested by the community

I'll just keep doing this...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, take two!

Committed and pushed to 8.5.x. Thanks!

webchick’s picture

Status: Fixed » Needs work

Ha, nevermind. It doesn't like:

yarn run v0.24.6
$ node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/8.x/core/misc/states.es6.js
[12:00:41] '/Users/angie.byron/Sites/8.x/core/misc/states.es6.js' is being checked.
✨  Done in 0.89s.

/Users/angie.byron/Sites/8.x/core/misc/states.es6.js
  41:43  error  Don't make functions within a loop  no-loop-func

✖ 1 problem (1 error, 0 warnings)
GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
FileSize
89.88 KB
1.32 KB

- fixes a minor coding standards issue in states.es6.js

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I'm constantly surprised by what babel is capable of.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Third time's a charm..? :P

Committed and pushed to 8.5.x. Thanks! (??!?)

  • webchick committed f1e33ca on 8.5.x
    Issue #2925064 by drpal, dawehner, droplet, xjm, webchick, justafish: [1...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

larowlan’s picture

Issue tags: -JavaScript +JavaScript

Posthumously 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.