Integrate Prettier or ESLint Code Formatting

[msalsbery]

Continuing the discussion at #1840 ...

It would be nice to use a modern code formatter to enforce a style across the OpenSeadragon codebase.

We already use ESLint, which in addition to its wonderful code quality rules, also has many code formatting rules available. The sheer number and complexity of formatting rules is great because it allows a finely-tuned format ruleset to be defined. Some downsides to having so many formatting rules available is added complexity in configuration and more options to have differing opinions about.

A great alternative tool for code formatting is Prettier. Prettier is extremely opinionated, thus it has very few rule options. Prettier only does formatting, and it does it well - if you're fine with its mostly opinionated ruleset.

Thoughts? Opinions? Which should we use?

[msalsbery]

My opinions and arguments for using Prettier....

I'm a big fan of Prettier! Despite its extremely opinionated rule set and small list of configuration options, I have yet to see any formatting that I can't live with and wish I could turn off...to the point I'm actually using auto-fix-on-save/paste option in all my projects, something I never thought I'd use.

My preference is to use ESLint for reporting code quality issues, and Prettier for code formatting...here's some of the reasons why...

The latest versions of Prettier and ESLint play nicely together!

...and with minimal configuration and a couple handy plugins, complement each other nicely:

• eslint-config-prettier plugin "Turns off all rules that are unnecessary or might conflict with Prettier"
• eslint-plugin-prettier plugin "Runs Prettier as an ESLint rule and reports differences as individual ESLint issues"

Prettier honors .editorconfig!

From the docs: "If options.editorconfig is true and an .editorconfig file is in your project, Prettier will parse it and convert its properties to the corresponding Prettier configuration. This configuration will be overridden by .prettierrc, etc. Currently, the following EditorConfig properties are supported:

• end_of_line (Prettier: endOfLine)
• indent_style (Prettier: useTabs)
• indent_size/tab_width (Prettier: tabWidth)
• max_line_length (Prettier: printWidth)

This makes it simple to have different indentation styles per file type without losing editorconfig's great generic support across all major code editors.

Prettier formats a bunch of file types, not just code!

Out of the box there's formatting for:

• JavaScript (including experimental features)
• JSX
• Angular
• Vue
• Flow
• TypeScript
• CSS, Less, and SCSS
• HTML
• JSON
• GraphQL
• Markdown, including GFM and MDX
• YAML

[iangilman]

From #1840:

I believe the way Prettier works is that it takes its cues from the ESLint config...

It does not ;)

OK, you've inspired me to figure out what's going on with my setup, because it definitely "works on my machine™"! Sure enough, I'm not just using Prettier after all… I'm using this plug-in for Atom:

https://atom.io/packages/prettier-atom

And I have this option turned on:

Anyway, since that's how I'm set up, that's what feels natural to me. I'm actually not sure how it would be with just Prettier. I do imagine there's a lot that Prettier is doing (when it comes to complex formatting) that ESLint wouldn't be able to do on its own.

So… I dunno, maybe we should start by going with just Prettier and see how it goes? If there's some style that's bugging us, we can always add eslint --fix.

[msalsbery]

@iangilman That's actually excellent info, thank you! You'll (hopefully) be pleased to know that my preferred proposal uses exactly what you're using!

First of all, I coincidentally just looked at the prettier-eslint package today, and although I personally don't need it in my projects, it's nice to know it's there if I do start seeing something that Prettier is doing that I don't like and I need fine-grained formatting rules that only ESLint has.

Second, since you're already using the ESLint/Prettier combination, you probably already have the configuration you like, which was what I was going to ask you for next. So if you like your setup, would like the same or similar for OpenSeadragon, and can share it, then could you please post your .eslintrc and .prettierrc contents here?

Edit: And your preferred .editorconfig please?

[msalsbery]

@iangilman To clarify and for my own notes...

The ESLint plugins that I use implement code formatting rules like I proposed above:

• eslint-config-prettier Turns off all rules that are unnecessary or might conflict with Prettier
• eslint-plugin-prettier Runs Prettier as an ESLint rule and reports differences as individual ESLint issues

This combination uses only Prettier's code formatting rules and lets ESLint just do code analysis (linting). Since Prettier has very few options, this combination is easy to configure, but forces the opinionated rules of Prettier.

The plugin you're using for Atom with the option you have selected uses this ESLint plugin:

• prettier-eslint formats your code via prettier, and then passes the result of that to eslint --fix

That plugin uses its own Prettier install if you haven't explicitely installed it locally or globally with npm. It works similar to the pair of plugins described above, but instead of turning off ESLint's formatting rules, it makes the pass with Prettier first, then ESLint fixes with its rules. With ESLint's much larger rule set, this plugin allows way more fine-grained control.

For general purpose across code editors like Atom, VS Code, and Sublime Text 3, it's easiest to use config files in the project root folder: .editorconfig, .eslintrc, and .prettierrc. (Note: The latest Prettier versions honor the following settings in .editorconfig: end_of_line, indent_style, indent_size/tab_width, and max_line_length)

I'm not sure how you configure the rules for the Atom plugin, but if you could please provide the formatting configuration rule settings you'd like to use for OpenSeadragon, I'll put together configs. Your experience on Atom shouldn't change at all, and should work seamlessly on at least VS Code and Sublime.

I hope that makes more sense than the barrage of text I posted above from my notes :)

[iangilman]

I'm happy to go with your proposed plug-in set up. How does this work for someone new on the project? Do they need to install something extra in their editor, or can they do it all from the command line if they want to? Editor integration is nice, but it should be optional.

I suppose we need to think about how it integrates into Travis as well.

Here are the config files from a recent project of mine. The editorconfig and prettierrc are pretty slim, of course. The eslintrc probably has a bunch of junk I don't actually care about… It's easy for it to pile up. To be honest, I probably started with the OpenSeadragon one and kept tweaking until nothing annoyed me anymore. Anyway, here goes:

editorconfig

# EditorConfig: http://editorconfig.org

root = true

[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

prettierrc

{
  "printWidth": 100,
  "semi": true,
  "singleQuote": true,
  "trailingComma": "none"
}

eslintrc

{
  "parser": "babel-eslint",
  "env": {
    "browser": true,
    "node": true
  },
  "parserOptions": {
    "ecmaVersion": 6,
    "sourceType": "module",
    "ecmaFeatures": {
      "jsx": true
    }
  },
  "extends": "eslint:recommended",
  "rules": {
    "no-console": 0,
    "indent": ["error", 2],
    "linebreak-style": ["error", "unix"],
    "quotes": ["error", "single"],
    "semi": ["error", "always"],
    "no-unused-vars": 0,
    "block-scoped-var": ["error"],
    "consistent-return": ["error"],
    "curly": ["error", "all"],
    "eqeqeq": ["error"],
    "no-eval": ["error"],
    "no-implicit-globals": ["error"],
    "no-implied-eval": ["error"],
    "no-multi-spaces": [
      "error",
      {
        "exceptions": {
          "Property": true,
          "VariableDeclarator": true,
          "AssignmentExpression": true
        }
      }
    ],
    "no-new-wrappers": ["error"],
    "no-new": ["error"],
    "no-return-assign": ["error"],
    "no-self-compare": ["error"],
    "no-unmodified-loop-condition": ["error"],
    "no-unused-expressions": ["error"],
    "no-useless-call": ["error"],
    "no-useless-concat": ["error"],
    "no-useless-escape": ["error"],
    "no-useless-return": ["error"],
    "no-with": ["error"],
    "radix": ["error"],
    "yoda": ["error"],
    "no-undef-init": ["error"],
    "no-use-before-define": [
      "error",
      {
        "functions": false
      }
    ],
    "array-bracket-spacing": ["error", "never"],
    "block-spacing": ["error"],
    "brace-style": ["error"],
    "comma-spacing": ["error"],
    "comma-style": ["error"],
    "computed-property-spacing": ["error"],
    "eol-last": ["error"],
    "func-call-spacing": ["error"],
    "func-name-matching": ["error"],
    "key-spacing": [
      "error",
      {
        "mode": "minimum"
      }
    ],
    "keyword-spacing": ["error"],
    "max-statements-per-line": [
      "error",
      {
        "max": 1
      }
    ],
    "new-parens": ["error"],
    "no-array-constructor": ["error"],
    "no-mixed-operators": [
      "error",
      {
        "groups": [
          ["&", "|", "^", "~", "<<", ">>", ">>>"],
          ["==", "!=", "===", "!==", ">", ">=", "<", "<="],
          ["&&", "||"],
          ["in", "instanceof"]
        ]
      }
    ],
    "no-new-object": ["error"],
    "no-tabs": ["error"],
    "no-unneeded-ternary": ["error"],
    "no-whitespace-before-property": ["error"],
    "object-curly-spacing": ["error", "always"],
    "one-var-declaration-per-line": ["error"],
    "operator-assignment": ["error"],
    "operator-linebreak": ["error", "after"],
    "quote-props": ["error", "as-needed"],
    "semi-spacing": ["error"],
    "space-before-blocks": ["error"],
    "space-before-function-paren": ["error", "never"],
    "space-in-parens": ["error", "never"],
    "space-infix-ops": ["error"],
    "space-unary-ops": [
      "error",
      {
        "words": false,
        "nonwords": false
      }
    ],
    "unicode-bom": ["error"],
    "no-caller": ["error"],
    "no-loop-func": ["error"],
    "padding-line-between-statements": [
      "error",
      { "blankLine": "always", "prev": "multiline-block-like", "next": "*" },
      { "blankLine": "always", "prev": "multiline-const", "next": "*" },
      { "blankLine": "always", "prev": "multiline-expression", "next": "*" },
      { "blankLine": "always", "prev": "multiline-let", "next": "*" },
      { "blankLine": "always", "prev": "multiline-var", "next": "*" }
    ]
  },
  "globals": {
    "Promise": false
  }
}

The Babel/ES6/JSX stuff is just what that project uses of course and isn't appropriate for OpenSeadragon.

The padding-line-between-statements item is something I've recently added, because I like blank lines after multiline statements, but I understand that's not a universal preference, so it seems reasonable to not include that.

Anyway, take it all with a grain of salt and let me know what you think! :-)

[msalsbery]

@iangilman Excellent, thank you!

No editor requirements for the user, although I will update the CONTRIBUTING doc with tips on editor integrations that'll help out! I'll add grunt tasks for CLI linting/prettying.

Travis...I don't know a thing about it. In what ways could a new config file (.prettierrc) and editing two existing config files (.editorconfig, .eslintrc.json) affect Travis? (serious question, not trying to be a smart-alec ;-))

[iangilman]

Yeah, it shouldn't affect Travis if we don't want it to. I believe Travis marks a build as failed if it doesn't pass the lint test. That's probably all we care about (in addition to the normal testing, of course), so there's nothing to add there.

Here's a recent Travis build, in case you're curious what it's doing:

https://travis-ci.org/github/openseadragon/openseadragon/builds/703626509

[msalsbery]

Oh yeah ;) Out of habit I always run eslint from command line when pushing PR so I didn’t think about that

[iangilman]

I've been falling behind on doing code review, so there are a lot of pull requests pending. I'm thinking maybe we should get those through the pipeline before doing anything that changes the code formatting, since it would likely bring a bunch of merge conflicts into all of those pull requests.

I'll see if I can find some time to start reviewing again… Of course any you want to do is also great, @msalsbery!

[msalsbery]

I've been falling behind on doing code review, so there are a lot of pull requests pending. I'm thinking maybe we should get those through the pipeline before doing anything that changes the code formatting, since it would likely bring a bunch of merge conflicts into all of those pull requests.

Oh geez, yeah good idea!

I'll see if I can find some time to start reviewing again… Of course any you want to do is also great, @msalsbery!

Cool, I'll see what I can do!