accordproject / techdocs

Accord Project Documentation
Apache License 2.0
61 stars 134 forks source link

AP ESLint Config #308

Open jolanglinais opened 4 years ago

jolanglinais commented 4 years ago

Feature Request πŸ›οΈ

Create an AP ESLint configuration to be shared among our repositories

Use Case

We should have our own ESLint configuration to help keep code across our ecosystem consistent and approachable

Possible Solution

This should run during tests to enforce linting

Context

Use @clausehq/eslint-config as a starting point reference

jeromesimeon commented 4 years ago

We may need different linting rules for core part of the stack and web components

jeromesimeon commented 4 years ago

Somewhat different issue, in the area of code checks:

jolanglinais commented 4 years ago

Yes maybe we could have a core stack config and a ui stack config?

jeromesimeon commented 4 years ago

Yes maybe we could have a core stack config and a ui stack config?

I'm not super versed in linter configurations, but one difference is the JavaScript style where the core stack uses CommonJS "requires" v the web components using the ES6 style.

Also the standard indentation in the core stack is 4 whitespace rather than 2 in the web components (I wouldn't mind getting rid of that difference though).

Aniruddha-Shriwant commented 3 years ago

Hey, I'm Interested in this :) I'm a newcomer here, I was just able to close this simple issue :) #341 I want to discuss this issue a little bit, I had joined Slack today, can you tell me to whom should I approach and discuss this issue there?

jolanglinais commented 3 years ago

Happy to discuss this more @Aniruddha-Shriwant, I know others can also help: @DianaLease @mttrbrts

jolanglinais commented 3 years ago

I think we should use the @clauseHQ/eslint-config ESLint configuration as a starting point. But I think we should be making our own configuration repository. One for β€œcore” and one for β€œui” to start with. So, @accordproject/eslint-core and @accordproject/eslint-ui. We could make this a monorepo (accordproject/linting?), which would make future linting configurations easier to add.

@jeromesimeon as a side note, this makes me think we may want to think about having a more structured naming convention for repositories and projects.

Aniruddha-Shriwant commented 3 years ago

Hey @irmerk , I apologize for the delay, I will make a sample .eslintrc.json / eslint config file keeping the below suggestion in mind

I'm not super versed in linter configurations, but one difference is the JavaScript style where the core stack uses CommonJS "requires" v the web components using the ES6 style. Also the standard indentation in the core stack is 4 whitespace rather than 2 in the web components (I wouldn't mind getting rid of that difference though).

and then we can discuss it and move on to further monorepo creation and so on...

Aniruddha-Shriwant commented 3 years ago

Hello @irmerk , I had created a .eslintrc.json file here as a sample, So that we can discuss some more rules that we should add in it... For convenience I'm pasting that file below...

{
    "extends": [
      "@clausehq/eslint-config",
      "eslint:recommended",
      "plugin:import/errors",
      "plugin:react/recommended",
      "plugin:jsx-a11y/recommended"
    ],
    "rules": {
      "react/prop-types": 0,
      "indent": ["error", 2],
      "linebreak-style":1
    },
    "parser": "babel-eslint",
    "plugins": [
        "react", 
        "import", 
        "jsx-a11y"
    ],
    "parserOptions": {
      "ecmaVersion": 2021,
      "sourceType": "module",
      "ecmaFeatures": {
        "jsx": true
      }
    },
    "env": {
      "es6": true,
      "browser": true,
      "node": true
    },
    "settings": {
      "import/resolver": {
        "node": {
            "paths": ["src"],
            "extensions": [".js", ".jsx", ".ts", ".tsx"]
        }
      },
      "react": {
        "version": "detect"
      }
    }
}
  1. The import plugin helps ESLint catch commons bugs around imports, exports, and modules in general
  2. jsx-a11y catches many bugs around accessibility that can accidentally arise using React, like not having an alt attribute on an img tag.
  3. react is mostly common React things, like making sure you import React anywhere you use React. babel-lint allows ESLint to use the same transpiling library, Babel, that Parcel uses under the hood. Without it, ESLint can't understand JSX.
  4. eslint-plugin-react now requires you to inform of it what version of React you're using. We're telling it here to look at the package.json to figure it out.
  5. Also as you had given me the starting point @clausehq/eslint-config I had also extended that.
  6. I had added the indentation rule of 2 whitespaces as suggested by @jeromesimeon , we can add other rule for core-stack in different file once the monorepo is created...

@DianaLease & @mttrbrts , I would like to know your views and opinions on this as you were the contributors in @clausehq/eslint-config...

mttrbrts commented 3 years ago

Thanks @Aniruddha-Shriwant for your efforts here.

@irmerk why don't we just adopt an existing lining ruleset? There are plenty to choose from and we avoid the overhead of maintaining our own.

jolanglinais commented 3 years ago

I'm going off the assumption that @jeromesimeon is correct in that we need two separate linters.

Aniruddha-Shriwant commented 3 years ago

Okay @irmerk , I didn't make two files before because I was waiting for your views on this file...will make other file soon :)

jolanglinais commented 3 years ago

@Aniruddha-Shriwant I think we'll want more input from @jeromesimeon on this before moving forward.

Aniruddha-Shriwant commented 3 years ago

@Aniruddha-Shriwant I think we'll want more input from @jeromesimeon on this before moving forward.

Yeah sure! Waiting for your reply @jeromesimeon πŸ˜…

Aniruddha-Shriwant commented 3 years ago

@irmerk any update on this? I had created two sample files... Core stack Web Components Looking forward to having your input on this...

Aniruddha-Shriwant commented 3 years ago

@irmerk pinging you here to discuss a little about configurations... Following are the .eslintrc.json files which can be treated as starter for the configurations

Sample .eslintrc.json for Accord Project (Core stack) Sample .eslintrc.json for Accord Project (Web Components)

Also,

I'm not super versed in linter configurations, but one difference is the JavaScript style where the core stack uses CommonJS "requires" v the web components using the ES6 style. Also the standard indentation in the core stack is 4 whitespace rather than 2 in the web components (I wouldn't mind getting rid of that difference though).

As suggested by Jerome, I had added these rules in those files...Also you may get a little information in this comment about various plugins I had added

Looking forward to having your suggestions on adding some specific rules which I missed adding and then we can move on creating Accords ESlint Config repo and further npm publishing work...

mttrbrts commented 3 years ago

I'm going off the assumption that @jeromesimeon is correct in that we need two separate linters.

Can we take an off-the-shelf linter for each pattern? I still don't understand the motivation for creating one or more custom house styles.

Each configuration might reference multiple existing configurations e.g 'airbnb/base', 'plugin:jest/recommended', 'plugin:react/recommended', however, I encourage us to avoid selecting individual rules.

mustang-shr commented 3 years ago

hey can i get to work on this issue?

jolanglinais commented 2 years ago

@mustang-shr I suggest following @mttrbrts's suggestion and implement an existing linter.

subhajit20 commented 8 months ago

hey @irmerk can you please elaborate on the problem do I have to create an eslint.config.js file inside the project or do I have to separately create one eslint config repo which will be shared across the other projects?