ESLint added, integration with prettier (need testing, suggestions) by runofthemillgeek · Pull Request #104 · microsoft/join-dev-design

runofthemillgeek · 2018-07-22T23:34:13Z

Overview of changes

Removed pretty-quick, upgraded husky to latest
Added ESLint, extends Airbnb's JS config
Uses lint-staged to run linting/prettifying on precommit
Uses prettier-eslint-cli to run Prettify and ESLint
Tweaked VS Code workspace settings to prevent HTML formatting (not entirely), integrate prettier-eslint
Use npm run fix:all to try lint/fix all the files
Runs npm install before npm run serve to ensure everyone is getting hooks working

Suggestions and Testing

I would've loved to discuss these in an issue but since there is no option to make one, I'd like to ask for help to see if this config is fine and if it works as currently configured. I don't know the extent to which the current setup can enforce the rules. Also, I have a few questions:

Has anyone faced issues with Prettier/ESLint when set to auto format/fix your changed files when executed? If not, is the current setup susceptible to any such issues? I'm not really well acquainted with doing so, especially when running both.
A possible solution for linting and fixing HTML? Currently, there is none and that's causing inconsistencies when it is touched in different commits.
Currently, HTML files are being fixed only when contributors are using editorconfig-aware editors and it is causing inconsistencies and accidental changes due to formatting. So, is there a way to fix it on precommit? (given there is no good html formatter)
Should we have an npm run dev instead of npm start? We could then prolly have CLI linting messages and possibly more dev stuff happening.

What do you guys think? Any suggestions? Also, I believe #55 could be merged soon and if we are planning to merge this, I hope the former gets merged first to avoid conflicts.

cc @dvdsgl

dvdsgl · 2018-07-22T23:48:48Z

Looking great!

runofthemillgeek · 2018-07-23T00:06:27Z

@dvdsgl Thanks. 😄 But I still wanna double/triple check whether it's working as intended. Do you have any suggestions regarding this and is Airbnb's style fine for JS?

ctrlaltvikas

Great!

prichodko

Thanks for a PR! 😄 It adds a lot of useful things, however I would love to see some changes before landing it.

I would love to see ESLint be concerned only about catching bugs and Prettier about formatting.

use Prettier (not prettier-eslint-cli) and after eslint --fix
add eslint-config-prettier to eslint.rc

Please also see my comments in the review. Let me know if anything is not clear. 😊

prichodko · 2018-07-23T09:14:03Z

.eslintrc.json

@@ -0,0 +1,8 @@
+{
+  "extends": "airbnb-base",


I would prefer simple and less opinionated eslint:recommended in this project.

Also I would leave all formatting on Prettier without any ESLint overrides, so if you add https://github.com/prettier/eslint-config-prettier as next config it overrides any rules that Prettier uses.

@prichodko Yeah, I think eslint:recommended would be a far less aggressive pick. Hm, I didn't pick eslint-config-prettier because it might class with Airbnb but if we're going to lean more on Prettier, then it's desirable I guess.

Yeah, that's why I am trying to make a clear distinction between ESLint (catching bugs) and Prettier (code style), because it makes the whole configuration much easier.

prichodko · 2018-07-23T09:18:23Z

package.json

+    "serve": "live-server docs",
+    "prettier": "prettier",
+    "eslint": "eslint",
+    "precommit": "lint-staged"


New versions of Husky has a different configuration - https://github.com/typicode/husky#install

Ah, you're right. I wonder why it still worked for me though.

Probably will be deprecated in future major version bump.

prichodko · 2018-07-23T09:20:28Z

package.json

+    "precommit": "lint-staged"
+  },
+  "lint-staged": {
+    "docs/**/*.js": [


Lint-staged runs regex on a list of staged files, this is why *.js is enough.

Roger. Will fix that.

prichodko · 2018-07-23T09:28:20Z

package.json

+  "lint-staged": {
+    "docs/**/*.js": [
+      "prettier-eslint --write",
+      "eslint",


Please use just Prettier and leave all formatting up to it.

You are suggesting it better to do ESLint -> Prettier? There are some good reasons why the opposite is better: prettier/prettier-eslint#101 (comment)

Not sure if I follow, but comment you are referring to seems to be more in favour of Prettier (also see https://twitter.com/kentcdodds/status/913760103118991361?lang=en).

Let me make my position clear: I am a big fan of the value that Prettier tries to bring (as few code-style discussion as possible), leaving a space for ESLint to modify code-style could potentially open (unnecessary) discussions concerned about personal preferences.

prichodko · 2018-07-23T09:28:23Z

package.json

-    "start": "live-server docs",
-    "precommit": "pretty-quick --staged",
-    "prettier": "prettier"
+    "start": "npm install && npm run serve",


This change is unrelated to this PR and also makes start slow. It's a common habit to start server/app with npm start and waiting for installing new node_modules is unnecessary. New node_modules should be done by user when opening a project or pulling changes from a remote not on every script start.

I totally agree with you on that. Only problem is, we need to ensure everyone who pulls new commits are doing npm install before carrying out their work. This is fine for project deps as that'll result in errors but dev deps — not so. For eg, installing husky adds hooks into .git/hooks/pre-commit but if someone forgot to do npm install after husky was added, they won't get the hooks — effectively bypassing the checks. This happened multiple times before when Prettier was added and some commits with incorrect styles got merged.

That's why I was wondering we do npm run dev in instead. I know the hosting provider usually takes care of doing npm install on changes and runs npm start so another npm install is unnecessary/might slow it a bit.

Gotcha'.

I agree with you, that it's definitely undesirable situation (which would be usually handled on CI for example). In our case I would still prefer rejecting PR rather than make changes to scripts.

Sangeeth Sudheer added 4 commits July 23, 2018 02:35

fix: runs npm i before starting up server

befa8c3

removed pretty-quick, added lint-staged

0e64500

added prettier-eslint and eslint config

44775f5

lint: fix all linting/formatting issues

5dcb67f

ctrlaltvikas approved these changes Jul 23, 2018

View reviewed changes

prichodko suggested changes Jul 23, 2018

View reviewed changes

Conversation

runofthemillgeek commented Jul 22, 2018

Overview of changes

Suggestions and Testing

Uh oh!

dvdsgl commented Jul 22, 2018

Uh oh!

runofthemillgeek commented Jul 23, 2018

Uh oh!

ctrlaltvikas left a comment

Choose a reason for hiding this comment

Uh oh!

prichodko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prichodko Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prichodko Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

prichodko Jul 23, 2018 •

edited

Loading

prichodko Jul 23, 2018 •

edited

Loading