Skip to content
This repository has been archived by the owner. It is now read-only.

Consider relaxing lint rules #333

Open
gaearon opened this Issue May 31, 2018 · 38 comments

Comments

Projects
None yet
@gaearon
Copy link
Contributor

gaearon commented May 31, 2018

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

This comment has been minimized.

Copy link
Owner

wmonk commented May 31, 2018

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

This comment has been minimized.

Copy link
Contributor Author

gaearon commented May 31, 2018

@filipdanic

This comment has been minimized.

Copy link

filipdanic commented May 31, 2018

Happy to take a crack at this. :)

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented May 31, 2018

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

@alexkuz

This comment has been minimized.

Copy link

alexkuz commented May 31, 2018

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

This comment has been minimized.

Copy link

anantoghosh commented May 31, 2018

I can work on this.

@filipdanic

This comment has been minimized.

Copy link

filipdanic commented May 31, 2018

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

This comment has been minimized.

Copy link
Collaborator

DorianGrey commented May 31, 2018

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

This comment has been minimized.

Copy link
Contributor Author

gaearon commented May 31, 2018

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

This comment has been minimized.

Copy link
Contributor

ianschmitz commented Jun 4, 2018

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

This comment has been minimized.

Copy link
Owner

wmonk commented Jun 7, 2018

@anantoghosh are you working on a PR for this?

@anantoghosh

This comment has been minimized.

Copy link

anantoghosh commented Jun 7, 2018

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

@codepunkt

This comment has been minimized.

Copy link

codepunkt commented Jun 14, 2018

@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:
tslintbail

Do you know otoh why that is?

@DorianGrey

This comment has been minimized.

Copy link
Collaborator

DorianGrey commented Jun 14, 2018

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

@codepunkt

This comment has been minimized.

Copy link

codepunkt commented Jun 15, 2018

While this seems to be the case, i think it goes hand in hand with adjusting tslint configuration. I don't want this to stop my development server, but a warning in the editor of choice and/or the build process would be fine so we could decide how to deal with this in CI environments.

@DorianGrey

This comment has been minimized.

Copy link
Collaborator

DorianGrey commented Jun 15, 2018

I don't want this to stop my development server

Feel free to do so - just disable the compiler option in tsconfig.json and enable it in tsconfig.prod.json (separation for this is supported since 2.16).
This option is part of the strict mode - even though that flag is not enabled by default atm., most parts of it are. As far as I know, using as many parts of it as possible is recommended.

but a warning in the editor of choice and/or the build process

There is a major difference between the linter and the compiler:
The linter can be leveled down to emit warnings in general or at least for particular rules, the compiler always emits errors in case of rule violations, thus you can only enable or disable them.
A suggestion for emitting warnings instead of errors was already made, but the issue doesn't seem to be that alive:
Microsoft/TypeScript#14501
Editors or IDE can only pick up one tsconfig.json, i.e. either for dev or prod, not both - thus, for a particular rule, you will either have a full-featured error, or nothing at all.

@codepunkt

This comment has been minimized.

Copy link

codepunkt commented Jun 15, 2018

@DorianGrey let's find a reasonable way around that limitation, then. It's not helpful to not see any warnings in development and have it break in production build on CI later on.

Visual Studio code for example, underlines an unused variable with a green warning line when noUnusedLocals in your tsconfig.json is set to true.

  • Is there a way to configure the development builds in webpack.config.dev.js so that they use tsconfig.json, but override that rule there?
  • Can we add an additional appTsDevConfig path to the config/paths.js file and use that in webpack.config.dev.js and then have an additional tsconfig.dev.json extend tsconfig.json the same way that the prod config does, but set noUnusedLocals to false in the dev version?
@ianschmitz

This comment has been minimized.

Copy link
Contributor

ianschmitz commented Jun 15, 2018

You could also use the tslint equivalent and leave the compiler option disabled.

@codepunkt

This comment has been minimized.

Copy link

codepunkt commented Jun 15, 2018

@ianschmitz sounds like the better alternative. Which tslint option is that?

@ianschmitz

This comment has been minimized.

Copy link
Contributor

ianschmitz commented Jun 15, 2018

This is the closest one: https://palantir.github.io/tslint/rules/no-unused-variable/. However note that because it requires type info, if you're using the VSCode TSLint extension, it doesn't display linting errors that require type info. See https://github.com/Microsoft/vscode-tslint/blob/master/tslint/README.md#faq

@gnapse

This comment has been minimized.

Copy link
Contributor

gnapse commented Jun 20, 2018

Warning: typescript newbie here.

Relaxing rules in tslint.json does not work. I got a compile error for an empty function block (which BTW seriously?) It's weird because I did not get the error in vscode, but maybe it has to do with this being in a javascript file, not a typescript file. But when I run yarn start I got the error as a compile error.

This is crazy. Preventing the app from even compiling because of a lint rule about a empty function block, or another lint rule about keys in an object literal not being sorted alphabetically is crazy.

But well, back to my issue: when I got the problem about the empty function block, I disabled that rule in the tslint.json file. But I still get the compile error. Then I changed my empty functions to be () => undefined instead of () => {}. This satisfied the linter, and now I get the "object literal keys not sorted" error, also as a compile error. At this point I refused to continue changing my code to satisfy these crazy rules, and I just removed tslint:recommended from the extend part of the tslint.json file. Problem "solved".

My point is, if there's something really useful in that tslint:recommended set of configs, I lost them. but that's what'll happen to most people that start to get frustrated about all these other rules in there that are frankly too much to force on everyone.

IMO, if this is supposed to be a sister project to CRA, it should provide a similar initial linting experience than CRA. That is, warn only about things that are really clearly not good. Unused variables, unreachable code, things like that. And also, please, do not make them compile errors when possible. Just warnings in the browser console and the terminal.

Other than all that, great initiative, typescript is awesome!

@ianschmitz

This comment has been minimized.

Copy link
Contributor

ianschmitz commented Jun 20, 2018

@gnapse refer to my comment above at #333 (comment)

@gnapse

This comment has been minimized.

Copy link
Contributor

gnapse commented Jun 20, 2018

Thanks! That solves the compile errors. But what about the fact that setting the rule as false did not work, but fixing the code to satisfy the rule did work? To be clear, I had linting errors initially on .tsx files, and as soon as I switch them off in the tslint.json file, the editor (vscode) removes the warning in the code. But with javascript files, I never got a warning in code, I only get the compile error, and switching the rule off still gives me the compile error. Seems like the linting performed by the editor is not the same as the linting performed during npm run start.

@sw-yx

This comment has been minimized.

Copy link

sw-yx commented Jun 30, 2018

just writing in from the /r/reactjs subreddit where i have been helping people get started with react + typescript. here's someone with 2 years exp in react running in to this issue: https://www.reddit.com/r/reactjs/comments/8o5owb/react_typescript_cheatsheet_for_react_users_using/e1jzh2x/?context=3

@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Jul 2, 2018

I think our most beginner friendly option is to switch back to CRA's ESLint config with typescript-eslint-parser. TSLint could potentially be removed if we take this approach, though it could still be used manually with tslint --project .. I'd be happy to help implement this approach if people agree.

Benefits

  1. The linter should behave similarly to CRA's config to reduce confusion, and it has few restrictions on Flow type usage (only annotation syntax).
  2. CRA's linter setup shows all linter warnings and errors together, making them easier to fix.
  3. Most of CRA's rules are only warnings, while the current setup often breaks production builds because all rules produce errors by default. Beginers think they have to make changes like booleanProp={true} in order to use TypeScript with React, they're actually opinionated recommendations from TSLint.
  4. The default TSLint config has no rules that use type info, but typescript-eslint-parser supports TypeScript metadata in parsed ASTs, so we can port our existing TSLint rules in theory.
  5. Only 15 rules of the enabled TSLint rules are specific to TypeScript, though they are not necessary to use TypeScript effectively.
  6. Many users of CRA-TS are porting from CRA, and don't know what TSLint is. If we used ESLint, we could simply enable it on both JS and TS files, and existing ESLint editor plugins would work. With the current TSLint setup, users would also have to install TSLint plugins for editor integration separately. TSLint also does not support enabling rules for both JS and TS files automatically, making tooling usage inconsistent when porting existing projects to TS.
  7. CRA's ESLint config has many important warnings that CRA-TS is missing, like accessibility support.
@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Jul 3, 2018

Here's my attempt using CRA's ESLint config with TypeScript ESLint Parser: Replace TSLint with CRA's ESLint config

@ianschmitz

This comment has been minimized.

Copy link
Contributor

ianschmitz commented Jul 3, 2018

@nickmccurdy this is a fairly significant change for this project. All your points make sense and are totally valid, but i think we should carefully consider how we would plan to support all the existing CRA-TS projects if we made this change.

@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Jul 3, 2018

@masaeedu

This comment has been minimized.

Copy link

masaeedu commented Jul 6, 2018

@gaearon I think the critical issue in the tweet you linked is

Just tried the typescript create-react-app starter and hit this error preventing me from doing anything.

"Import sources within a group must be alphabetized."

I'm sorry, but people ... come on ... are you serious?

Similarly, from another tweet:

Everything is an error all the time perfectly sums up how I learned to hate strict typing.

The problem is people are having an experience of linting/typechecking that the TypeScript compiler itself is designed to avoid. TypeScript will emit JS just fine so long as your code is syntactically valid; you're free to enable highly opinionated rules like noUnusedGlobals, because you can still compile your code and run your tests. Developing with typescript isn't a constant process of linearly walking down a list of type errors and fixing them one by one before anything will work at runtime.

The tool should provide some option that makes it imitate the behavior of tsc in this respect, i.e. the app can be started up and live reloaded when TypeScript emit succeeds, regardless of semantic errors.

@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Jul 6, 2018

That's a good point, I think CRA-TS needs a way to allow builds without TS passing. However, I still feel it's best to switch to ESLint for parity with CRA.

@whut

This comment has been minimized.

Copy link

whut commented Jul 26, 2018

There is an important comment from @nickmccurdy posted on #360:

I believe you can use "defaultSeverity": "warning" in tslint.json for now.

I can only add that this should be added by default to tslint.json generated by create-react-app-typescript.

@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Jul 26, 2018

If that's a simple non-breaking change, I think we should do it now. However I'm working on removing TSLint anyway, so that would no longer be an issue if my work is merged.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Sep 24, 2018

Has this stalled? It’s still kind of brutal.

https://mobile.twitter.com/monicalent/status/1043780023730221056

@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Sep 24, 2018

Awaiting review: #388

Alternatively we may want to recommend existing CRA users to upgrade to CRA 2 for TS

@ianschmitz

This comment has been minimized.

Copy link
Contributor

ianschmitz commented Sep 24, 2018

Agree with @nickmccurdy. Would love to see facebook/create-react-app#4837 make it in, then this fork could be deprecated.

@mikew

This comment has been minimized.

Copy link

mikew commented Oct 23, 2018

Is there more to this than changing the default lint severity to "warning" and moving some more strict tsconfig.json options to tsconfig.prod.json?

@gnapse

This comment has been minimized.

Copy link
Contributor

gnapse commented Oct 23, 2018

@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Oct 23, 2018

Note that CRA's TS support hasn't been release yet, but it is merged into their master branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.