Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP ESLint Config #308

Open
jolanglinais opened this issue Sep 10, 2020 · 21 comments
Open

AP ESLint Config #308

jolanglinais opened this issue Sep 10, 2020 · 21 comments
Labels

Comments

@jolanglinais
Copy link
Member

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
Copy link
Member

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

@jeromesimeon
Copy link
Member

Somewhat different issue, in the area of code checks:

  • license checks on all project

@jolanglinais
Copy link
Member Author

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

@jeromesimeon
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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

@jolanglinais
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

Aniruddha-Shriwant commented Feb 28, 2021

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
Copy link
Member

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
Copy link
Member Author

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

@Aniruddha-Shriwant
Copy link
Contributor

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
Copy link
Member Author

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

@Aniruddha-Shriwant
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

Aniruddha-Shriwant commented Mar 20, 2021

@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
Copy link
Member

mttrbrts commented Mar 23, 2021

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
Copy link

hey can i get to work on this issue?

@jolanglinais
Copy link
Member Author

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

@subhajit20
Copy link

subhajit20 commented Mar 9, 2024

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?

@MominHussain123
Copy link

get to work on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants