thoughtbot / eslint-config

A sharable ESLint configuration that enforces thoughtbot’s JavaScript guides.
https://thoughtbot.com
MIT License
10 stars 1 forks source link

Update ESLint configurations, add configs for React web, React Native, TypeScript #1

Closed stevehanson closed 1 year ago

stevehanson commented 1 year ago

This PR adds configurations for React, React Native, TypeScript, and web without React, and updates the default configuration to be React.

This PR takes an opinionated approach intended to match how we write JavaScript and TypeScript at thoughtbot. Therefore, the configs include plugins where appropriate like eslint-plugin-react-hooks, eslint-plugin-jsx-a11y, eslint-plugin-react-native-a11y, eslint-plugin-jest, and eslint-plugin-testing-library, and prettier.

The PR is a starting point. I'd love to get feedback and refine it collaboratively to get to where we want it to be.

Example usage of this package is to run yarn add --dev @thoughtbot/eslint-config and then add the following to the ESLint config file (eg. .eslintrc.js):

{
  extends: [
    "@thoughtbot/eslint-config", // or eg. `@thoughtbot/eslint-config/native`
    "@thoughtbot/eslint-config/typescript"
  ]
}

See the readme in this PR for more info.

Current State

Our current ESLint configuration hasn't been updated since 2020 and includes only formatting rules. The thoughtbot guides currently advise to turn off all ESLint formatting rules and allow Prettier to handle formatting, making this entire package as it exists today irrelevant. From the guides:

If ESLint is used along with Prettier, the ESLInt plugin eslint-config-prettier should also be used to turn off all ESLint style rules that are already handled by Prettier.

Airbnb as the base

Most of the base rules that were added in this PR (files in the rules directory) are copied over from Airbnb's ESLint config. The defaults seem to be a good starting place and are widely used. I've found that most organizations I've worked with are already using Airbnb's config internally and usually am asked to use that as a base. The only main divergences in the core rules are in rules/overrides.js, rules/native.js, and rules/typescript.js. I expect us to diverge more and refactor how the rules are set up in this PR, but this seemed like a good first step.

Besides that, this package includes many other ESLint plugins that Airbnb does not (eg. Jest, Testing Library, and Prettier) and is easier to setup as it only requires installing this single package as opposed to Airbnb's config requiring each plugin to be manually installed.

Airbnb's config is very large, so I do think there is opportunity for us to simplify our config and not include everything. There are also a lot of style/formatting rules included which all get turned off anyway when we include the prettier plugin. Some of our exported configs (eg. base) don't include Prettier, so I thought it would be good to keep the style rules included.

I could also be convinced to not export base at all and have every exported config use Prettier and therefore not include any style/formatting rules in this repo. I think that would be much simpler to maintain.

Call for additional testing

I have mostly tested the React Native configuration. I was able to switch to this package on my current project that was using Airbnb without issue. It would be helpful if other folks could try dropping this into their project to see how it works for them. This way we can identify any rules that might be a problem.

To test this out, from your project directory, run yarn add thoughtbot/eslint-config#eslint-configs and then remove any eslint packages from your package.json other than eslint itself, as they are no longer needed. Finally, update your .eslintrc.js (or similar) config file based on the instructions in the readme, and then run ESLint and see what errors you get!

Other configs for reference

stevehanson commented 1 year ago

Thank you for taking a look, @whitecl! I'm happy to hear that the process went smoothly. Was there any particular reason you left eslint-import-resolver-typescript and eslint-plugin-import in your package.json? Those should both be installed automatically by this script, so I was wondering if you ran into any issues there.

Also, are you still using eslint-import-resolver-babel-module? I didn't see the usage in your .eslintrc.json, but it looks like it's in your package.json so was curious.

One last question: it looks like you left parser and plugins defined. Was there any specific error you were getting if you left those out? This config should have covered those for you as well.

Thanks again!

whitecl commented 1 year ago

@stevehanson I was able to remove eslint-import-resolver-typesciprt and eslint-plugin-import from package.json without any issues. Nice!

I also removed eslint-import-resolver-babel-module - I had an empty object configuring it in my .eslintrc and removal resulted in no changes, so is probably a relic from the time I spent troubleshooting eslint configuration on this project.

You're also right about parser and plugins - I was able to remove those without any issue. 👍

Here is how much I was able to simplify things - this is a huge improvement!

diff --git a/assets/.eslintrc b/assets/.eslintrc
index 1ff0fab..aae2faf 100644
--- a/assets/.eslintrc
+++ b/assets/.eslintrc
@@ -1,13 +1,8 @@
 {
   "extends": [
-    "airbnb",
-    "prettier",
-    "plugin:@typescript-eslint/recommended",
-    "plugin:react/recommended",
-    "plugin:react/jsx-runtime"
+    "@thoughtbot/eslint-config",
+    "@thoughtbot/eslint-config/typescript"
   ],
-  "parser": "@typescript-eslint/parser",
-  "plugins": ["@typescript-eslint", "jsx-a11y", "import"],
   "rules": {
     "complexity": ["warn", 5],
     "import/extensions": 0,
@@ -24,16 +19,5 @@
     "react/require-default-props": 0,
     "camelcase": 0,
     "@typescript-eslint/no-unused-vars": ["error"]
-  },
-  "settings": {
-    "import/parsers": {
-      "@typescript-eslint/parser": [".ts", ".tsx"]
-    },
-    "import/resolver": {
-      "babel-module": {},
-      "typescript": {
-        "alwaysTryTypes": true
-      }
-    }
   }
 }
diff --git a/assets/package.json b/assets/package.json
index 3c40eb3..aa438c6 100644
--- a/assets/package.json
+++ b/assets/package.json
@@ -40,6 +40,7 @@
     "webpack-cli": "^4.9.2"
   },
   "devDependencies": {
+    "@thoughtbot/eslint-config": "github:thoughtbot/eslint-config#eslint-configs",
     "@types/ramda": "^0.27.66",
     "@types/react": "^17.0.45",
     "@types/react-dom": "^17.0.17",
@@ -48,15 +49,7 @@
     "copy-webpack-plugin": "^7.x",
     "css-minimizer-webpack-plugin": "^3.x",
     "eslint": "^8.39.0",
-    "eslint-config-airbnb": "^19.0.4",
-    "eslint-config-prettier": "^8.3.0",
     "eslint-import-resolver-babel-module": "^5.3.2",
-    "eslint-import-resolver-typescript": "^3.5.5",
-    "eslint-plugin-import": "^2.27.5",
-    "eslint-plugin-jsx-a11y": "^6.7.1",
-    "eslint-plugin-prettier": "^3.4.0",
-    "eslint-plugin-react": "^7.24.0",
-    "eslint-plugin-react-hooks": "^4.2.0",
     "mini-css-extract-plugin": "^1.x",
     "prettier": "^2.3.2"
   }
stevehanson commented 1 year ago

I just added ESLint to a new CLI project (private thoughtbot repo) and used this config. This was my first attempt to use the "base" config, and I ran into a small issue where some React rules were being turned on that I pushed up a commit (07b311a) to address.

My config for this project looks like:

{
  "extends": [
    "@thoughtbot/eslint-config/base",
    "@thoughtbot/eslint-config/prettier",
    "@thoughtbot/eslint-config/typescript"
  ],
  "rules": {
    "no-console": "off"
  }
}
adriantoddross commented 1 year ago

I just spun up a new Next.js app with TypeScript. Extending my eslint config with "@thoughtbot/eslint-config/base" works but I get the following error when trying to use "@thoughtbot/eslint-config/typescript":

Failed to load config "@thoughtbot/eslint-config/typescript" to extend from.
stevehanson commented 1 year ago

@adriantoddross thanks for taking a look! I haven't been able to reproduce this error yet. Can you share some more info on how you installed this package and configured ESLint? Can you verify that you have the latest version of this branch installed? Also, what version of ESLint are you using?

Here is the diff of what I did on a new Next.js app, which uses ESLint 8.44.0, and it seems to be working for me:

.eslintrc.json

 {
-  "extends": "next/core-web-vitals"
+  "extends": [
+    "next/core-web-vitals",
+    "@thoughtbot/eslint-config/base",
+    "@thoughtbot/eslint-config/prettier",
+    "@thoughtbot/eslint-config/typescript"
+  ]
 }
package.json

    "react": "18.2.0",
    "react-dom": "18.2.0",
    "typescript": "5.1.6"
+  },
+  "devDependencies": {
+    "@thoughtbot/eslint-config": "thoughtbot/eslint-config#eslint-configs"
  }
}

Also, is there a reason you aren't extending the React ESLint config? The base config won't include our React rules. Instead of base and prettier, you can just do:

"extends": [
  "next/core-web-vitals",
  "@thoughtbot/eslint-config", // this loads the React config by default, or @thoughtbot/eslint-config/react is the same thing
  "@thoughtbot/eslint-config/typescript"
]
adriantoddross commented 1 year ago

Thanks for the quick reply @stevehanson!

I installed the package with npm install @thoughtbot/eslint-config --save-dev. I stuck with Next's default ESLint config through create-next-app and don't have ESLint installed globally.

Also, is there a reason you aren't extending the React ESLint config?

Sorry, I made a typo there; My current .eslintrc.json is:

{
  "extends": [
    "next/core-web-vitals",
    "@thoughtbot/eslint-config"
    "@thoughtbot/eslint-config/typescript"
  ],
}

Can you verify that you have the latest version of this branch installed?

I think this is the issue I'm having. In my package.json, my version of @thoughtbot/eslint-config is 0.1.0instead of thoughtbot/eslint-config#eslint-configs!

  "dependencies": {
    "next": "13.4.10",
    "react": "18.2.0",
    "react-dom": "18.2.0",
    "typescript": "5.1.6"
  },
  "devDependencies": {
    "@thoughtbot/eslint-config": "^0.1.0", /
    "eslint-config-prettier": "^8.8.0",
    "prettier": "^3.0.0"
  }
}

Since this is more troubleshooting than PR review, I can move this convo to Slack if you'd like!

adriantoddross commented 1 year ago

I've got my issue resolved. I installed the original package instead of this branch!

stevehanson commented 1 year ago

@adriantoddross aha, that makes sense! Thanks for trying this out and providing the update. Glad it's working for you now 🎉