chore: improve eslint rules and configuration

[nickofthyme]

Summary

Add industry-standard best practices around javascript from airbnb and others using recommended rulesets.

See all changes to the config in these commits https://github.com/elastic/elastic-charts/pull/689/files/21ef4de978687c9f53b5ad35e4cdf8d9d6c4a916

rules for @monfera's consistent-function-scoping and class-methods-use-this
rules for @markov00 explicit-module-boundary-types and no-use-before-define

Added plugins:

• eslint-config-airbnb-typescript
• eslint-plugin-eslint-comments
• eslint-plugin-header
• eslint-plugin-import
• eslint-plugin-jest
• eslint-plugin-jsx-a11y
• eslint-plugin-promise

Notable changes

• Top-level javascript config files will now be linted
• Ordered imports
• Add lint-staged to run only staged files on commit-msg. This will not add the changes to the commit.
• Upgrade existing eslint plugin packages
• Upgrade eslint package

With the newly added rules, there is a pretty large diff but most were fixable. You can scroll through the changes in this commit 8300f99

5701 problems (5544 errors, 157 warnings)
  4711 errors and 11 warnings potentially fixable with the `--fix` option.

🚧Disclaimer 🚧

I am open to discussion on any issues anyone may have with these new rules now or in the future.

I think the rules that we would like to have but maybe too hard to fix right now, we can add warnings to fix them over time.

airbnb prohibits the use of for of loops which requires ignoring each one until finding a better way to allow them or decide to enforce not using them.

[markov00]

I've left some preliminary comments, In general we should carefully check:

• the default switch case (is should be placed in the right place)
• a header and a internal annotation are stripped out, we should carefully check this
• I've some doubts on some for loops replaced by for of
• I've some doubts on some DOM API replacement

Anyway this is a great and awesome PR I love all the changes applied

[markov00]

That's a great PR, thank you Nick for going through this!
All my comments are resolved, everything seems to align perfectly to our library coding philosophy
❤️

[rshen91]

There looks to be a large amount of awesome changes in this! Thanks for doing this
I'm a fan of including the jsx-a11y plugin and the import rules. I didn't see any rules at the error 2 level that I didn't agree with. I hadn't seen the eslint enable and disable comments before (such as in the theme_service.ts file) so that was cool to see.

[nickofthyme]

@rshen91 I'll hold off on merging this until you get your color contrast PR merged. That way you don't have to fuss with all the conflicts.

[monfera]

I'm super happy to OK with most rules; some code linked comments are added. This PR is definitely a broad sweeping code improvement, thanks for this!

My single change request is the shedding of the import reorder rule, it is annoying and useless in practice, based on former use of it, would it be possible to remove it unless there's some specific value I should learn about. I don't want to cause extra work, so no problem if import order in the PR remains what this rule yielded, it's just unpleasant to have this rule, going forward, as it replaces the writer's potentially informed order with random order.

So prettier will be pretty much gone with this, right? As some indentation etc. rules seem to be different.

[monfera]

Sure there's some order (heavily dependent on .../ count) but it's no better than random. Meanwhile, if ordering is left to the coder, the ordering reflects some kind of logic, which is helpful. On the left, the constant imports are below each other; on the right, it's semantically random, and lookup is not any better. So it feels overconstraining without bringing in value. Source: past, painful experience with enforced input order :-) I can be convinced if it's important for most folks, or if we chat about actual benefits ofc :-)

[nickofthyme]

See changes in 55df0db. I will keep the changes to the files but relax the rule.

[monfera]

Thanks Nick! Wouldn't it be useful to allow new lines? Doesn't matter with 2-3 imports, but if it's more than 5 or so, sometimes conceptual grouping via empty line is helpful, eg. framework stuff - constants - utils - ... Unless someone thinks there's utility to a wall of imports, it could be enabled. I personally find some grouping useful

[nickofthyme]

I could see the utility in that but the grouping, unless uniform over all files, is arbitrary to everyone else but the original author.

If you were to group like this then later someone other than the code author will add an import, not knowing the grouping, the grouping are now both arbitrary to everyone else and inconsistent (per the original grouped intent).

I think we try it this way for now and see about changing it if you or others are annoyed by it. I think we should still separate the thrid-party at the top regardless of local import grouping/order.

[monfera]

Not sure what the motive for this one is, because the benefit is, saving only 2 spaces, the cost is, the indentation will not reflevt the ternary structure, and making ternaries easily readable is an important goal. If it's coming from a ternary-specific indentation rule I wouldn't mind not adding this rule, or at least I hope not to receive PR comments in the future that hinge on suboptimal ternary indentation :-)

[nickofthyme]

It comes from a general indent rule, which many configurable options including this case. See my thoughts on your other related comment here #689 (comment).

[monfera]

Are we changing row length? If not, what was the problem with the previous one? This feels more noisy, and there's more semantic load on the code reader. Current version: ah this line is just a property, sure, happens to be a flat object. With this rule: ah we have an opening curvy, let's take a breath, make a coffee and dive into what lurks beneath this nested structure 😄

[nickofthyme]

yes and no. The prettier print-width option was replaced with the max-len in eslint using the same 120 character limit. However, this is only set to warn, at least for now, and will not fix anything automatically.

This change is related to the object-curly-newline rule which strives for consistent line returns within a given object, not the entire file.

{
  ...
  fontShape: { fontStyle: 'normal', fontVariant: 'normal', fontWeight: '900', fontFamily: 'sans-serif' },
}

{
  ...
  fontShape: {
    fontStyle: 'normal', fontVariant: 'normal', fontWeight: '900', fontFamily: 'sans-serif'
  }, // <-- requires consistent "outer" brackets
}

{
  ...
  fontShape: {
    fontStyle: 'normal',
    fontVariant: 'normal', // <-- requires consistent "inner" brackets (i.e. for each element)
    fontWeight: '900',
    fontFamily: 'sans-serif'
  }, // <-- requires consistent "outer" brackets
}

So this allows the flexibility to use any style, so long as you are consistent on a local level.

[nickofthyme]

This change was just a remnant from tweaking the styles. I reverted to the original here 89275be.

[monfera]

If TS has no complaints, I don't either 👍

[nickofthyme]

Yeah same. I think these come from past code that required it but then types were improved and this becomes verbose and unnecessary. The rule should only complain in that case and not if the typecasting is justifiable.

[monfera]

Is there an automated rule that overwrites a substr with a slice? I'd like to learn; used to think the former shows clearer intent with strings

[nickofthyme]

Yup prefer-string-slice with auto fix.

Souce: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/v20.1.0/rules/prefer-string-slice.js

[monfera]

This would need some realignment, or if the poor autoformatting is deemed OK, the // prettier-ignore should be removed

[nickofthyme]

89275be

[monfera]

Still no fan of indenting the closing parens at the same level as where the outer structure markers ? and : are

[nickofthyme]

This seems to be the most logical and consistent to me.

Everywhere else we match the closing indent to the indent of the opening tab line. If you were to change the ternary indentation with your suggestion, then all other indenting should match that, in which case that would look something like...

const someValue = isTrue
  ? { // <-- match this plus 1 indent unit for }
    a: 'one',
    b: 'two',
    } // <-- 0 + 1 indent 
  : { // <-- match this plus 1 indent unit for }
    a: 1,
    b: 2,
    }  // <-- 0 + 1 indent  

const someObject = { // <-- match this plus 1 indent unit for }
  a: 'one',
  b: 'two',
  } // <-- 0 + 1 indent 

const someArray = [ // <-- match this plus 1 indent unit for ]
  'one',
  'two',
  ] // <-- 0 + 1 indent 

const someFn = () => { // <-- match this plus 1 indent unit }
  // do something
  } // <-- 0 + 1 indent 

Wouldn't you agree it is more consistent? Anyway if you'd like me to change that I will.

[monfera]

Proposal in the PR:

return node.depth < treeHeight
  ? {
    x0: node.x0,
    y0: node.y0px,
    x1: node.x1,
    y1: node.y0px + getTopPadding(topGroove, node.y1px - node.y0px),
  }
  : {
    x0: node.x0,
    y0: node.y0px,
    x1: node.x1,
    y1: node.y1px,
  };

My slant:

return node.depth < treeHeight
  ? {
      x0: node.x0,
      y0: node.y0px,
      x1: node.x1,
      y1: node.y0px + getTopPadding(topGroove, node.y1px - node.y0px),
    }
  : {
      x0: node.x0,
      y0: node.y0px,
      x1: node.x1,
      y1: node.y1px,
    };

Pros:

• the structure of the ternary is clearer - I don't know about others, but with peripheral vision, which is the only thing we have when looking at the top, it's easy to jump to the next outdent, which will be the true 2nd branch (: part) with my ask and a noise (in this context) character } otherwise
• it's easier for the eye to match { with } for an object because they have the same indentation; both the { and the } are in unconstrained rows
• it's immediately clear if you glance at : that you return a flat object one way or the other, because the closing } is exactly where the next opening { is
• reasonable indent ("tab") count:
  1. level: ternary structure ? and : (and nothing else)
  2. level: what's inside each ternary (happens to be an object, starting/ending with {}/{} but could be something else that's also multirow, eg. a function call, or a long algebraic expression)
  3. level: object properties
• there's increased clarity as the contained props are indented relative to the opening delimiter { - sure it's slightly redundant, but code formatting is about redundancy, and accessibility for developers means that we should take into account diverse age groups, experience levels or dyslexia; likely, more marked structuring appears to be more important to me than than to you (as I'm the one asking for it), and a next developer contributing or joining may have an even stronger benefit from structural clarity that a simple rule can buy us

Cons:

• two spaces needed - not a real cons as usually we don't put super long lines as prop values
• maybe other things that haven't come up, that are important to you or someone else on the team?

I'm fine with whatever you conclude with @markov00 as I've no further thought on it and your version is already an improvement on the current ternary layout so it's a good change as it is 👍

[nickofthyme]

Ok I'm fine trying it out for now. Let's see if it grows on me 🌱

See e3ba904

[monfera]

Just reading: @markov00 what DOM API overwrites occur? I'm concerned about it unseen 😃 I missed it but I'm no fan of for loop rewrites into for ... of for various reasons, one of them is, there's the occasional but significant performance difference; the other one is, conveying the intent (sure the rewriter can latch onto special cases eg. const value = data[i]; but it doesn't automatically make it more appropriate)

[nickofthyme]

My single change request is the shedding of the import reorder rule, it is annoying and useless in practice, based on former use of it, would it be possible to remove it unless there's some specific value I should learn about. I don't want to cause extra work, so no problem if import order in the PR remains what this rule yielded, it's just unpleasant to have this rule, going forward, as it replaces the writer's potentially informed order with random order.
...
Sure there's some order (heavily dependent on .../ count) but it's no better than random. Meanwhile, if ordering is left to the coder, the ordering reflects some kind of logic, which is helpful. On the left, the constant imports are below each other; on the right, it's semantically random, and lookup is not any better. So it feels overconstraining without bringing in value. Source: past, painful experience with enforced input order :-) I can be convinced if it's important for most folks, or if we chat about actual benefits ofc :-)

So my biggest grip with import order is the grouping of different types of imports. Mainly third-party and internal. This just makes it easy for me to see what third party things are imported and keeps somewhat of an order.

I don't really care about alphabetical or otherwise order within those groups so I understand your point there. However, looking through the code I don't really see any sensible order to most imports. I think that having a structure to how they are ordered in a consistent way that is automatically fixable by your IDE is reasonable. Ideally I think the order within these groupings should be based on the path directory such that it's easy to see import from neighboring directories. For example the imports below seem out of place.

import { GOLDEN_RATIO } from '../../../partition_chart/layout/utils/math';
import { ShapeViewModel } from '../../layout/types/viewmodel_types'; // <-- this import seems out of place
import { cssFontShorthand } from '../../../partition_chart/layout/utils/measure';

I don't think there is a way to specify order within a group other than alphabetize but maybe we can do this a later date if that is something you agree with.

All that being said I removed the ordering within the groups and refined the groups to be builtin, external and internal. See 55df0db.

So prettier will be pretty much gone with this, right? As some indentation etc. rules seem to be different.

Correct, I want to see how we fare without using prettier. I just came across too many conflicts related to prettier/eslint. Also, I would say eslint is able to replicate about 90% of prettier options. Prettier just is too inflexible in terms of configuration that makes it so difficult. I adapted the replacement rules from this article that enumerates most of the pain points.

It is not perfect, specifying related to indenting. But this is in process on the eslint-typescript plugin.

If it turns out after some time that we miss prettier than I am happy to bring it back or possibly write my own eslint rule to make the changes we are missing.

Just reading: @markov00 what DOM API overwrites occur? I'm concerned about it unseen 😃 I missed it but I'm no fan of for loop rewrites into for ... of for various reasons, one of them is, there's the occasional but significant performance difference; the other one is, conveying the intent (sure the rewriter can latch onto special cases eg. const value = data[i]; but it doesn't automatically make it more appropriate)

Yup I removed that rule and now there is a rule to error on any use of for...of see discussion here #689 (comment).

[monfera]

Thanks @nickofthyme for the changes and this fantastic explanation, I learned things from it, and I'm fine with the alphabetical ordering too, so you can skip reading this 😄

So my biggest grip with import order is the grouping of different types of imports. Mainly third-party and internal. [...]

Looks like we want a similar outcome

Ideally I think the order within these groupings should be based on the path directory such that it's easy to see import from neighboring directories.

It's good you wrote it down because I haven't had this wish but maybe it'd grow on me? In general it sounds useful to have a gradient of local to global. Not sure how it works in practice, eg.

• D3 is global, but we often use it through shallow wrapper utils (so we can easily replace each as needed), which makes it more local, still, those are just wrappers
• for some reason I felt it's neat to separate constants (and start with them), then types, then utils etc. though I haven't labored much on the order :-)

For example the imports below seem out of place:

import { GOLDEN_RATIO } from '../../../partition_chart/layout/utils/math';
import { ShapeViewModel } from '../../layout/types/viewmodel_types'; // <-- this import seems out of place
import { cssFontShorthand } from '../../../partition_chart/layout/utils/measure';

It's from the alpha goal charts which still makes sideways references, in any case the logic here (if any) was an order of constants -> types -> functions. Local directory depths are slightly random

As said on the top, it's fine for me either way as you gave some good reasons (eg. distance based orderint) I wasn't thinking of

[monfera]

Approving this with the updates and/or argument given. Unless there are counterarguments, or too much work involved, I wouldn't mind the indenting I described in one of the threads

[codecov-commenter]

Codecov Report

Merging #689 into master will decrease coverage by 0.14%.
The diff coverage is 76.08%.

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
- Coverage   73.27%   73.13%   -0.15%     
==========================================
  Files         271      271              
  Lines        8847     8784      -63     
  Branches     1747     1747              
==========================================
- Hits         6483     6424      -59     
+ Misses       2313     2309       -4     
  Partials       51       51              

Impacted Files	Coverage Δ
...art_types/goal_chart/layout/viewmodel/viewmodel.ts	3.12% <0.00%> (ø)
.../chart_types/goal_chart/state/selectors/tooltip.ts	41.66% <ø> (ø)
src/chart_types/index.ts	100.00% <ø> (ø)
..._types/partition_chart/layout/circline_geometry.ts	98.43% <0.00%> (ø)
src/chart_types/partition_chart/layout/geometry.ts	77.77% <ø> (ø)
...types/partition_chart/layout/types/config_types.ts	100.00% <ø> (ø)
...c/chart_types/partition_chart/layout/utils/math.ts	100.00% <ø> (ø)
...hart_types/partition_chart/layout/utils/measure.ts	100.00% <ø> (ø)
...tion_chart/layout/viewmodel/hierarchy_of_arrays.ts	88.88% <ø> (ø)
src/chart_types/specs.ts	100.00% <ø> (ø)
... and 275 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eded2ac...2bd7874. Read the comment docs.

[nickofthyme]

@monfera I couldn't find any plugin to achieve the directory locality gradient sorting so I added back the alphabetical group sort here ddae736. If it gets to be too much I am fine removing it.

[markov00]

🎉 This PR is included in version 19.5.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀