Open gaearon opened 6 years ago
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.
For inspiration, here is the CRA lint config:
https://github.com/facebook/create-react-app/blob/next/packages/eslint-config-react-app/index.js
Happy to take a crack at this. :)
In particular, note that we don't use ERROR
for anything that is not likely completely broken.
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.
I can work on this.
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.
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.
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.
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"
}
@anantoghosh are you working on a PR for this?
@wmonk No, I thought @filipdanic is working on it.
@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?
Don't mix tslint
errors with those from typescript
- the error above if from the latter, known as noUnusedLocals
.
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.
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:
https://github.com/Microsoft/TypeScript/issues/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.
@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
.
webpack.config.dev.js
so that they use tsconfig.json
, but override that rule there?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?You could also use the tslint equivalent and leave the compiler option disabled.
@ianschmitz sounds like the better alternative. Which tslint option is that?
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
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!
@gnapse refer to my comment above at https://github.com/wmonk/create-react-app-typescript/issues/333#issuecomment-394448666
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
.
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
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.
booleanProp={true}
in order to use TypeScript with React, they're actually opinionated recommendations from TSLint.Here's my attempt using CRA's ESLint config with TypeScript ESLint Parser: Replace TSLint with CRA's ESLint config
@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.
My pull request disables TSLint but will not error if a config still
exists. Do you want full back compatability for existing configs? If so we
could keep TSLint enabled when the config is present and avoid generating
it in new projects. Otherwise we can provide a warning when a config
exists, recommending users run tslint --project .
manually.
Either way I think we should make the presence of the more relaxed and consistent config the default for new projects and make it obvious that it exists for existing users.
On Mon, Jul 2, 2018, 9:09 PM Ian Schmitz notifications@github.com wrote:
@nickmccurdy https://github.com/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.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wmonk/create-react-app-typescript/issues/333#issuecomment-401982538, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4l9C-ON7JIgcCYCsh4W8I8-Ve5S1KTks5uCsREgaJpZM4UU1o5 .
@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.
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.
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.
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.
Has this stalled? It’s still kind of brutal.
https://mobile.twitter.com/monicalent/status/1043780023730221056
Awaiting review: https://github.com/wmonk/create-react-app-typescript/pull/388
Alternatively we may want to recommend existing CRA users to upgrade to CRA 2 for TS
Agree with @nickmccurdy. Would love to see https://github.com/facebook/create-react-app/pull/4837 make it in, then this fork could be deprecated.
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?
FYI, CRA now supports TypeScript: https://github.com/facebook/create-react-app#whats-included
Note that CRA's TS support hasn't been release yet, but it is merged into their master branch.
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.