Consider relaxing lint rules

[gaearon]

Hi folks!

We currently link to this project from React documentation. I haven’t tried it personally but I assumed it would follow the philosophy in CRA. In particular, we don’t use lint warnings for things that are inconsequential (code style) and only use them to warn people about actual bugs in their code. This was a major design decision after seeing beginners struggle with screens fully of linter errors about spacing and alphabetising props when they’re just learning a library!

However, I recently realized this project doesn’t actually subscribe to this philosophy, and has very strict lint rules:

https://twitter.com/ryanflorence/status/1002028375903354881

https://twitter.com/acemarke/status/1002029156492771328

I don’t want to debate usefulness of this, but I’m wondering how intentional it was. If it is intentional I think we’ll need to avoid recommending this setup for beginners, for the same reason we don’t recommend Airbnb config to them. But maybe it’s not that important and you can consider relaxing them to stay closer to CRA’s original vision?

Thanks for consideration.

[wmonk]

Hi! Thanks for stopping by. I just responded to the thread #329 addressing this. General gist is I'm happy for someone to make a copy of the rules from CRA to provide a more similar experience.

[gaearon]

For inspiration, here is the CRA lint config:

https://github.com/facebook/create-react-app/blob/next/packages/eslint-config-react-app/index.js

[filipdanic]

Happy to take a crack at this. :)

[gaearon]

In particular, note that we don't use ERROR for anything that is not likely completely broken.

[alexkuz]

I should also mention the problem I ran into in my project (I just started to use TypeScript recently): I had to add another TS config for production, since I don't want unused variables to be treated as errors in development (just as warnings), but on the other side, there is no way to build production version (or lint on precommit) with zero tolerance for warnings, there has to be an error.

[anantoghosh]

I can work on this.

[filipdanic]

The way it works now, rules aren’t loaded at all from packages/eslint-config-react-app (not present in webpack config at all), but rather just tslint.json.

This file includes the tslint:recommended preset which is very prohibitive. One way of solving this is to update tslint.json in the spirit of CRA’s philosophy. Or, to discard it and include the rules via webpack from the packages directory. The rules there are mostly already in-line with CRA’s own lint config.

@wmonk any preference here? I think that removing the tslint.json altogether might be more in line with the no-config philosophy. It’s one less file to worry about when you get started.

[DorianGrey]

Just keep in mind that the current tslint setup relies on several presets that include default values for almost every rule it can offer. Some of these rules and their values are more opinionated or useful than others (e.g. import-order vs jsx-no-lambda), yet none of them exists solely for "torturing" devs, but for providing an environment based on configurations that are considered good practice for both typescript and react.
Being confronted with additional, yet strict guidelines on "how to write your code" might be frustating for some while useful for others. It's impossible to cover both ends.
Personally, I think it is always preferable to be a bit forced to think about how to write code, and not just "get it working somehow", especially for beginners, since ... once you're familiar with messy code style, it will be even harder to get rid of it.

[gaearon]

Any person who thinks ordering imports is important probably already has opinions about TSLint config and will configure it themselves anyway. So we might as well relax the default for the people who don't necessarily find something like this "messy style" and just want to get the work done.

[ianschmitz]

A setting in tsconfig.json our team found super helpful is: "defaultSeverity": "warning".

This will report lint errors as warnings during development (npm start or npm run build), but when CI=true it will treat the lint warnings as errors.

For reference our tsconfig.json:

{
    "extends": ["tslint:recommended", "tslint-react", "tslint-config-prettier"],
    "linterOptions": {
        "exclude": ["config/**/*.js", "node_modules/**/*.ts"]
    },
    "rules": {
        "arrow-parens": false,
        "eofline": false,
        "interface-name": false,
        "jsx-boolean-value": false,
        "jsx-no-lambda": false,
        "jsx-no-multiline-js": false,
        "member-access": false,
        "no-return-await": false,
        "no-submodule-imports": false,
        "no-trailing-whitespace": false,
        "no-var-requires": false,
        "object-literal-sort-keys": false,
        "only-arrow-functions": false,
        "ordered-imports": false,
        "prefer-conditional-expression": false,
        "semicolon": [true, "always", "ignore-bound-class-methods"],
        "trailing-comma": false,
        "variable-name": [
            true,
            "ban-keywords",
            "check-format",
            "allow-leading-underscore",
            "allow-pascal-case"
        ]
    },
    "defaultSeverity": "warning"
}

[wmonk]

@anantoghosh are you working on a PR for this?

[anantoghosh]

@wmonk No, I thought @filipdanic is working on it.

[codepunkt]

@anantoghosh @filipdanic If you don't start this soon, i will. Don't wait for one another, just do it! 😛

@wmonk simply relaxing a few rules and tslint.json to defaultSeverity: "warning" will not be sufficient. The build pipeline, i assume it's somewhere in the webpack rules, also bails out on anything that tslint warns about.

Example:

Do you know otoh why that is?

[DorianGrey]

Don't mix tslint errors with those from typescript - the error above if from the latter, known as noUnusedLocals.