Lint with standardjs

[etpinard]

The standard set of linting rules are becoming the standard 😆 for open-source JS libraries.

We should follow suit.

[rreusser]

Strong choice! See also: standard-format

Edit: ah, never mind. Seems like standard --fix is now built in.

[etpinard]

I believe plotly.js should remain a ES5-only repository for the foreseeable future. I also believe that npm run lint should catch all non-ES5 slip-ups.

Eslint is configured with ES5-only parser by default (ref), but standard now uses an ES8 parser 😲 (ref) meaning that running the stock standard in plotly.js will not catch non-ES5 tokens.

So, I propose using standard-own which uses that same engine as standard but allows uses to specify which ECMA script to parse. I think adding:

{
  "standard-own": {
    "eslintConfig":  {
       "parserOptions": {
          "ecmaVersion": 5
        }
     }
  }
}

to the package.json should do the trick.

Alternatively, we could publish an es5 version of eslint-config-standard and use that?

[rreusser]

Interesting. Agreed on keeping it ES5 for the sake of consistency. (though es2020 hits the nail on the head, IMO.)

Related discussion: standard/standard#159

[rreusser]

Though I'm mostly trying to understand the consequences of the parser vs. the set of rules and what that means for simply enforcing style.

[etpinard]

Though I'm mostly trying to understand the consequences of the parser vs. the set of rules and what that means for simply enforcing style

Good point here. Perhaps we could just use standard to enforce style and add some small syntax test using acorn that would error out on ES6 tokens to enforce ES5 separately.

[rreusser]

Testing out ./node_modules/.bin/standard --fix now, just to watch things burn…

[etpinard]

To enforce ES5, we could use use-es5-only which simply errors out when trying to parse non-es5 tokens (w/o listing detail about the error) or try to write something better using esutils to provide better info about the non-ES5 token found.

[rreusser]

Ah, looks like that maybe just sets the parser version of eslint. Separate steps seem nice since they're two different things, but if it's really as simple as setting the parser version, maybe it's not worthwhile to separate it.

[etpinard]

Ah, looks like that maybe just sets the parser version of eslint.

As far as I know, I don't think one can set the parser version within standard.

[alexcjohnson]

FWIW @etpinard and I had a slack chat about styles a few weeks ago... we talked about how I'm freaked out by the restrictions imposed by no ;, how there are things like semistandard but it doesn't have much traction, etc etc... it concluded with me saying:

I could still be convinced to go with standard if someone can convince me it really is becoming the standard throughout the js dev community… until then I feel like we should stick with what we have.

So let me generalize that statement: if we change styles, it had better be because the community is truly coalescing around a style AND we're convinced it's going to last many years that way. My gut reaction is that we shouldn't switch styles at all unless and until we move to ES6+ (but please don't open that can of worms here!)

Also for the record I completely agree with the outcome of #1629 - while enforcing completely automated style has its advantages, if it stops you from doing things that - rules be damned - make your code more readable (extra parens, d3 half-indents, etc etc) - then it's going too far.

And of course @rreusser 's #1629 (comment) is spot on:

The golden rule of conventions: Convention <insert convention you use here> is aesthetically superior to <insert proposed alteration here>. 😉

So there's really no point arguing about what's superior on the merits. Unless you can start the discussion with "all the major active projects (in ES5) are using this style so it'll be easier for new people to adapt to our project," lets leave our style as it is now.

[monfera]

Entering the room, realizing that I missed the bikeshedding :-) Please therefore ignore my self-healing attempt :-)

I started liking no-semicolons with ES2015, as its streamlined syntax made the semicolons stick out, indeed it's subjective. I admittedly haven't considered majority share due to switching back and forth anyway eg. between plotly.js and gl-vis. Looks like standard.js is the most starred style but semi still leads in large libs. Maybe everybody is waiting for some other large libs to switch 😆

The sample list is neat and there are non-standardjs libs which omit semis eg. regl or bootstrap. Standard.js announced recently to audacity/ridicule, and revisiting now, I'm impressed to see a lot of libraries, react-: 322, or react-: 94 hits. It looks grassroots, and FB/React itself hasn't tipped over, though they just tipped over to rollup which was only used by D3 4.0 a short while ago. Subjective choices have more inertia than objectively better stuff :-)

A couple times I've run into a superficial and immediately obvious issue with an iife in ES5. With ES2015 there's hardly ever a need for an iife because of the block scoping of let/const that replace basically all vars. We avoid iife-s even with ES5 but if there's still a need I don't mind the preceding semi as it's also a warning sign (yeah Stockholm syndrome).

Re the D3 indentation convention, I used it when learning D3 but my editor kept destroying it, and I found it confusing to maintain . So I don't fluent down levels, just assign a variable to new scenegraph levels, it'll be compact uglified, but much easier to add a new 'enter'/'update'/'exit'/'filter'/'transition' without accidentally breaking a long chain. It adds maintainability with the large scenegraphs we have but a tad longer.

[monfera]

To add an example for overbearing format rules, we already have some constraints that feel a bit arbitrary, compare this, what I think would be nice but not allowed:

    var missing = function(n, i) {
        return traceIn.link.source.indexOf(i) === -1 
            && traceIn.link.target.indexOf(i) === -1;
    };

with this, as allowed by our current linter:

    var missing = function(n, i) {
        return traceIn.link.source.indexOf(i) === -1 &&
            traceIn.link.target.indexOf(i) === -1;
    };

Aside from nice vertical alignment, I like important things to not be at the line end if they can be at the beginning, as it shows intent and structure faster. I know most shops put their ternary operators to the end like this, and we have no intention to revisit it:

    tinycolor(layout.paper_bgcolor).getLuminance() < 0.333 ?
        'rgba(255, 255, 255, 0.6)' :
        'rgba(0, 0, 0, 0.2)';

but this shows the ternary nature more directly:

    tinycolor(layout.paper_bgcolor).getLuminance() < 0.333
        ? 'rgba(255, 255, 255, 0.6)'
        : 'rgba(0, 0, 0, 0.2)';

[rreusser]

Have to agree with @alexcjohnson. I still prefer some of the standard/semistandard conventions, but short of something drastic like prettier that I think does have merits beyond the aesthetic, I think bikeshedding is best avoided, especially if it involves hundreds of manual variable declaration reworkings just to get it off the ground.

Oh, and let's not forget the extra steps required to sort out git blame history as soon as you change the linting.

[etpinard]

eslint@4 is out: http://eslint.org/docs/user-guide/migrating-to-4.0.0

Note the changes to the indent rule. Running eslint . with v4 from the root of plotly.js/ spits out more than 4000 errors and unfortunately eslint --fix . doesn't do a great job fixing our code base.

[rreusser]

Could just disable…

To address: We recommend upgrading without changing your indent configuration, and fixing any new indentation errors that appear in your codebase. However, if you want to mimic how the indent rule worked in 3.x, you can update your configuration:

{
  rules: {
    indent: "off",
    "indent-legacy": "error" // replace this with your previous `indent` configuration
  }
}

[etpinard]

Probably won't happen anytime soon, closing.