Add Prettier and ESLint standard configs #5

msanroman · 2020-06-03T18:14:07Z

Purpose

Prettier

This is the standard Prettier config file for the projects within the https://github.com/bufferapp GitHub organization.

This was created by taking a look at Account, Publish, Authentication Service, Engage (backend), and Engage (frontend) and aiming for a minimal version that relies on Pretier defaults and uses the most common settings set already by our different teams.

There were repositories setting items with default values, such as tabWidth : 2 or bracketSpacing: true.

Eslint

This exposes two different ESLint configs: one for React frontends and another for Typescript backends. The aim here is to reduce external dependencies and have a minimal configuration that relies on standards based on the tools themselves rather than external companies (aka: eslint-airbnb).

These configurations omit any default setting for now.

Issues

Closes #1 and #2

esclapes

Hey @msanroman thanks for kick starting this work.

Regarding prettier, Core is only using it integrated with eslint via this plugin and config and not run independently for the following reasons:

eslint has two broad categories of rules: format and code check, while prettier only cares about format (and it's quite opinionated about it)
when working together it's possible to have rules conflicting, so eslint has a set of configs that help you disable the formatting rules from eslint and a plugin that allows you to check and fix formatting rules via prettier-eslint integration (basically eslint runs prettier under the hook).

If the next step in standardization is to have a common eslint setup, I'd find more value on bundling both eslint and prettier configuration in a single shared module.

However, I may be missing some cases where we will have prettier enabled and not eslint. If that's the case then this would be fine. 😄

esclapes · 2020-06-11T06:49:15Z

prettier/README.md

+> npm install --save-dev @bufferapp/prettier-config
+# or
+> yarn add -D @bufferapp/prettier-config


If prettier is a peer dependency it would be great to include it in the install command.

Yeeeees! 🙌

msanroman · 2020-06-11T12:31:11Z

Hey @msanroman thanks for kick starting this work.

Regarding prettier, Core is only using it integrated with eslint via this plugin and config and not run independently for the following reasons:

eslint has two broad categories of rules: format and code check, while prettier only cares about format (and it's quite opinionated about it)

when working together it's possible to have rules conflicting, so eslint has a set of configs that help you disable the formatting rules from eslint and a plugin that allows you to check and fix formatting rules via prettier-eslint integration (basically eslint runs prettier under the hook).

If the next step in standardization is to have a common eslint setup, I'd find more value on bundling both eslint and prettier configuration in a single shared module.

However, I may be missing some cases where we will have prettier enabled and not eslint. If that's the case then this would be fine. 😄

Yes! That's a very good one @esclapes . I was prepping an ESLint PR for kicking off the discussion for frontend and backend teams and sharing base settings between them, but maybe we can merge these into this.

Right now we have repos that don't have Prettier enabled at all, but the goal is to have both anywhere anyway 😄

msanroman · 2020-06-12T15:33:40Z

@esclapes - just added eslint as a different package for now. As far as I can understand reading https://eslint.org/docs/developer-guide/shareable-configs, there would be some resolution issues if we were to package both things together.

Still, I think with documentation (and if needed, scripting) we could automate the installation of both if needed.

CoxyBoris

Heya @msanroman thanks for kicking this off!

There is a bunch of options I needed to add for Engage in order to remove some false errors or to make jest compatible with this configuration. I wonder if we should include this as well in this PR?

CoxyBoris · 2020-06-19T05:38:29Z

eslint/backend.js

+    extends: [
+        "./index.js",
+        "plugin:@typescript-eslint/recommended",
+        "prettier/@typescript-eslint"


Curious if we should use "plugin:prettier/recommended" as well? We've used it on Engage to displays prettier errors as ESLint errors.

CoxyBoris · 2020-06-19T05:38:33Z

eslint/backend.js

+    plugins: [
+        "typescript-eslint",
+    ],
+    parser: "@typescript-eslint/parser",


Had to use these option to have the parser working correctly. Not sure it's still necessary:
"parserOptions": { "ecmaVersion": 2018, "sourceType": "module" },

CoxyBoris · 2020-07-08T11:30:43Z

Hey @msanroman ! 👋 Curious if you've seen my comments on the PR :) No rush at all!

philippemiguet

Hey @msanroman @CoxyBoris, as mentioned in #10, I have come up with some base configuration using what is in this repo + what was missing from core/engage that we find really useful.

What I have in my PR may represent and include the addition from Boris and Edu wanted to make to this PR.

Two questions:

How do you feel about updating this PR with the files I have in Create a base backend package with all the configuration files #10? I'm happy to own that 😊
How does it work when importing eslint config which requires installing packages? Do we simply need to tell engineers to install this package + all dependencies?

CoxyBoris · 2021-10-04T07:27:02Z

How do you feel about updating this PR with the files I have in #10? I'm happy to own that 😊

Sounds good to me thanks!!

How does it work when importing eslint config which requires installing packages? Do we simply need to tell engineers to install this package + all dependencies?

hmm shouldn't all dependencies be already listed in package.json and installed on npm install ? Sorry if I'm missing something here

philippemiguet · 2021-10-04T15:18:43Z

hmm shouldn't all dependencies be already listed in package.json and installed on npm install? Sorry if I'm missing something here

Yeah maybe! I have no knowledge about how this works, I will only know once I try it. Maybe declaring the dependencies in this package will be enough 🤔 @msanroman since you already researched that part, do you know?

msanroman · 2021-10-04T15:27:32Z

They should be declared as both devDependencies for working in the package and as peerDependencies for when you're installing the package as a dependency. That's how I've seen it work with Airbnb's ESLint config ¹

https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb/package.json ↩

msanroman · 2021-10-04T15:29:19Z

We can use this shortcut to make sure we install the peerDependencies alongside the package:

npx install-peerdeps --dev eslint-config-airbnb

The key thing is to make sure only the absolute needed is marked as peer deps :)

philippemiguet · 2021-10-04T15:59:16Z

Wow, I didn't know about the peerDependencies attribute 🤯

philippemiguet · 2021-10-07T14:31:34Z

backend/tsconfig.json

+    "target": "es6",
+    "module": "commonjs",
+    "declaration": true,
+    "outDir": "./lib",


todo: change this for built

philippemiguet · 2021-10-07T14:35:33Z

eslint/backend.js

+    "typescript-eslint",
+  ],
+  parser: "@typescript-eslint/parser",
+  ignorePatterns: ["node_modules/*", "built/*", "lib/*", "build/*"],


We should standardize our tsconfig outDir folder for all repositories. Right now, we have: lib, build, and built depending on repos. I would go for built 😊

built sounds good to me!

philippemiguet · 2021-10-07T14:36:00Z

backend/base-backend-package/.eslintrc.json

-        "jest/valid-expect": "error",
-        "no-underscore-dangle": "off"
-    }
+  "env": {


todo: replace with actual eslint package we are creating here

philippemiguet · 2021-10-07T14:36:08Z

eslint/backend.js

+  rules: {
+    "@typescript-eslint/no-unused-vars": "error",
+    "@typescript-eslint/explicit-function-return-type": "error",
+    "@typescript-eslint/explicit-module-boundary-types": "error",
+    "jest/no-disabled-tests": "warn",
+    "jest/no-focused-tests": "error",
+    "jest/no-identical-title": "error",
+    "jest/prefer-to-have-length": "warn",
+    "jest/valid-expect": "error",
+    "no-underscore-dangle": "off"
+  }


Tempted to add these rules to the index.js as they should be applied to both FE and BE :in my opinion.

philippemiguet · 2021-10-07T14:39:23Z

Listing next steps here @msanroman @CoxyBoris

Set rules to index.js to apply to both BE and FE
Add peerDependencies to eslint package
Decide on TS config outDir standard name: built vs build vs lib
Move tsconfig to a package: is that possible?
Update backend libs to use prettier and eslint package as a first example

esclapes reviewed Jun 11, 2020

View reviewed changes

msanroman changed the title ~~Add Prettier standard config~~ Add Prettier and ESLint standard configs Jun 12, 2020

CoxyBoris requested review from CoxyBoris and removed request for CoxyBoris June 19, 2020 05:29

CoxyBoris reviewed Jun 19, 2020

View reviewed changes

philippemiguet mentioned this pull request Oct 1, 2021

Create a base backend package with all the configuration files #10

Closed

philippemiguet reviewed Oct 1, 2021

View reviewed changes

msanroman added 4 commits October 7, 2021 16:11

Add prettier config v0.0.1

4f930b1

Add ESLint standard v0.0.1

ad2ac63

Add prettier installation on Usage

a5358a4

Add README.md

a6f5f90

philippemiguet force-pushed the task/add-prettier branch from eac9e1f to a6f5f90 Compare October 7, 2021 14:11

philippemiguet added 4 commits October 7, 2021 16:13

Add tsconfig standard for backend

e864f01

Add editor config to repo

9f2e811

Update prettier config using base-backend-package config

e55295a

Update eslint config using base-backend-package config

d741d06

philippemiguet reviewed Oct 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Prettier and ESLint standard configs #5

Add Prettier and ESLint standard configs #5

msanroman commented Jun 3, 2020 •

edited

Loading

esclapes left a comment •

edited

Loading

esclapes Jun 11, 2020

msanroman Jun 11, 2020

msanroman commented Jun 11, 2020

msanroman commented Jun 12, 2020

CoxyBoris left a comment

CoxyBoris Jun 19, 2020

CoxyBoris Jun 19, 2020

CoxyBoris commented Jul 8, 2020

philippemiguet left a comment

CoxyBoris commented Oct 4, 2021 •

edited

Loading

philippemiguet commented Oct 4, 2021

msanroman commented Oct 4, 2021

msanroman commented Oct 4, 2021

philippemiguet commented Oct 4, 2021

philippemiguet Oct 7, 2021

philippemiguet Oct 7, 2021

CoxyBoris Oct 11, 2021

philippemiguet Oct 7, 2021

philippemiguet Oct 7, 2021

philippemiguet commented Oct 7, 2021

Add Prettier and ESLint standard configs #5

Are you sure you want to change the base?

Add Prettier and ESLint standard configs #5

Conversation

msanroman commented Jun 3, 2020 • edited Loading

Purpose

Prettier

Eslint

Issues

esclapes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msanroman commented Jun 11, 2020

msanroman commented Jun 12, 2020

CoxyBoris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoxyBoris commented Jul 8, 2020

philippemiguet left a comment

Choose a reason for hiding this comment

CoxyBoris commented Oct 4, 2021 • edited Loading

philippemiguet commented Oct 4, 2021

msanroman commented Oct 4, 2021

Footnotes

msanroman commented Oct 4, 2021

philippemiguet commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippemiguet commented Oct 7, 2021

msanroman commented Jun 3, 2020 •

edited

Loading

esclapes left a comment •

edited

Loading

CoxyBoris commented Oct 4, 2021 •

edited

Loading