Replace TSLint with CRA's ESLint config

[nickserv]

This is an attempt to fix #333 by replacing TSLint with CRA's default ESLint setup for a more beginner friendly setup that is consistent with what JS users of CRA are used to. May contain breaking changes. See my in depth rationale here: #333 (comment)

Verification

• TSLint violations no longer appear (for example, removing public from App.tsx)
• ESLint warnings appear on TS files (for example, adding eval('string'))
• Linter passes on default template with yarn start and yarn build
• yarn test is unaffected
• ESLint behaves after ejecting

Tasks

• Document migration from TSLint
• Consider using eslint-plugin-typescript for TypeScript specific rules (currently JS rules run through a TS parser)
• Consider how to handle back compatibility (warning on tslint.json, major release, keeping TSLint and ESLint, etc.)
• Verify source maps

[paulmelnikow]

Hi, thanks for doing this work. I'm trying to use it, and I'm running into an issue where I want to run the linter from the command line. Do you know how to go about that? If I run eslint . it complains about not having a config file. If I create an empty config file I get parsing errors on existing .js files.

Is there a recipe for doing this in CRA that I should be following?

Happy to dig into this, but I could use a few pointers.

[nickserv]

Unlike CRA-TS, CRA does not generate an ESLint config and does not allow the internal config to be edited. The ESLint loader is built into commands like npm start and npm run build which you should run instead of eslint ., and it should behave exactly like CRA does (except with TS syntax support). I did not add any TS specific ESLint rules, but we may want to do that since Facebook has some similar rules for Flow in CRA.

[paulmelnikow]

Gotcha. Thanks. So to make sure it's lint free in CI, I just run npm run build.

What's the rationale for not being able to override the config? Currently I'm running into an issue where there are lots and lots of false-positive no-undef's on class properties. It's a similar issue to eslint/eslint#8720.

Added: Is there a way to trigger autofix using the scripts?

[nickserv]

What's the rationale for not being able to override the config?

This was CRA's decision, and I thought it would be simpler and less surprising to take the same decision. CRA's philosophy is that almost nothing is configurable, everything follows sane defaults.

Currently I'm running into an issue where there are lots and lots of false-positive no-undef's on class properties.

Oops I didn't notice that issue. They say it was related to class properties, do you think the issue could be that we're using the typescript parser instead of eslint/babel?

Is there a way to trigger autofix using the scripts?

No, but the default config is very minimal and has no style/syntax rules, so it probably wouldn't be useful anyway. ESLint can't really autofix most semantic rules, like defining used variables.

[paulmelnikow]

I ran into another false positive: no-unused-vars on an imported interface type.

This was CRA's decision, and I thought it would be simpler and less surprising to take the same decision. CRA's philosophy is that almost nothing is configurable, everything follows sane defaults.

It makes sense, especially if the rules are extremely basic and always problems, as you say. If I want to lint additional stuff I can install and run my own copy of eslint. It's a bit frustrating in this circumstance, but that's only because of the issue we're discussing. 😁

… do you think the issue could be that we're using the typescript parser instead of eslint/babel?

Not sure, though I could give this a try.

It's also possible that we should just turn off this rules on .ts and .tsx files.

By the way, I do occasionally get the build to run and the app to display in the presence of lint errors. Maybe one time out of five this happens. That's why I was looking to trigger it separately in CI. It's unrelated to these changes – i.e. I've seen it happen both before and after this change.

[paulmelnikow]

Related discussions:

no-unused-vars issue: https://github.com/nzakas/eslint-plugin-typescript/issues/33#issuecomment-400063994 bradzacher/eslint-plugin-typescript#90 bradzacher/eslint-plugin-typescript#112

no-undef issue: bradzacher/eslint-plugin-typescript/issues/110

[nickserv]

You're right, there are issues with no-undef and no-unused-vars after all. I'm trying to fix these as it was not intentional and I don't want to force people to eject just to avoid ESLint parser bugs.

I think I'll just disable no-undef on TS files since TS errors on undefined variables anyway, but the TypeScript parser and plugin for ESLint don't seem to have an easy way to fix no-unused-vars. The default TS config has noUnusedLocals already which is half of the no-unused-vars, so what if we enabled noUnusedParameters to be consistent with eslint-config-react-app? Then we can disable the no-unused-vars rule, but we have to be careful with users changing allowJs and checkJs.

In other news ESLint seems to be making progress on Babel 7 based TypeScript support, but I don't think it's ready yet. The only other issue I've noticed so far is that ESLint seems to confuse the <></> React fragment shorthand syntax with strings, and I get a no-multiline-string-error.

[nickserv]

Thanks, just skimmed those issues. I would much rather have the full type system support of noUnusedLocals and noUnusedParameters (which is much more stable) than have the ability to turn an error to a warning, especially since the current TSLint setup only shows one error at a time and we may need to make breaking changes anyway to switch people to TSLint.

I personally think disabling no-undef and no-unused-vars in TS is the way to go, but what should we do about checkJs? Should we enabled it by default and disable those rules in JS, or enable the rules in JS (which means that users enabling checkJs may see unused variable and undefined variable warnings twice, both in TS and ESLint)?

[paulmelnikow]

Thanks, just skimmed those issues. I would much rather have the full type system support of noUnusedLocals and noUnusedParameters (which is much more stable) than have the ability to turn an error to a warning, especially since the current TSLint setup only shows one error at a time and we may need to make breaking changes anyway to switch people to TSLint.

Agreed. Turning off no-undef and no-unused-vars in TS sounds right. It might be possible to do that simply by turning off typescript/no-undef and typescript/no-unused-vars.

One thing, though: -noUnusedParameters seems to conflict with https://github.com/facebook/create-react-app/blob/8a34b7cc77099c7f591d64726a8106c02104f8f8/packages/eslint-config-react-app/index.js#L134 which has args: 'none'.

Does the TypeScript compiler report these errors on JS files? I'm pretty new to all of the TypeScript tooling, though I'd be surprised if it did.

[nickserv]

Good catch, I thought CRA was using the default options for no-unused-vars. Unfortunately there's no exact alternative to after-unused in TS, but I think it would be fine to keep noUnusedParameters disabled by default since it's still configurable.

As far as I'm aware, noUnusedLocals and noUnusedParameters work on JS files if allowJs and checkJs are enabled. We currently only enable allowJs by default.

[paulmelnikow]

They're using none, not after-unused.

As far as I'm aware, noUnusedLocals and noUnusedParameters work on JS files if allowJs and checkJs are enabled. We currently only enable allowJs by default.

Sounds like you think it'll currently be reported by eslint but not the compiler. I'll check.

[paulmelnikow]

Confirmed, I'm getting it from eslint but not the compiler.

A lint error in a JS file seems to be a reproducible example of this issue:

By the way, I do occasionally get the build to run and the app to display in the presence of lint errors. Maybe one time out of five this happens. That's why I was looking to trigger it separately in CI. It's unrelated to these changes – i.e. I've seen it happen both before and after this change.

yarn build emits the warning but then runs to completion with a successful exit status. yarn start prints the warning in the console but the built site displays in the browser.

Think I should raise a new issue for that?

[nickserv]

Nevermind, this is consistent with CRA after all, it only shows errors in the browser overlay. The reason this didn't happen before is that the TS and TSLint loaders only reported errors, but most of eslint-config-react-app's rules are warnings.

[nickserv]

I couldn't get eslint-plugin-typescript to work as expected, so for now I'm just disabling rules I know to be problematic in TS files. Thanks for your help.

My CRA-TS app is now free of ESLint errors and warnings after disabling the false positives, but I'd appreciate if you could test this new commit and see if your apps still have any incorrect warnings/errors. Besides discovering more problematic rules, we should consider forking eslint-config-react-app to reduce duplication, make it easier to configure editor ESLint plugins (they can't read Webpack configs), and allow usage outside of CRA-TS (CRA also supports their ESLint config for non-CRA users).

[paulmelnikow]

Will do.

forking eslint-config-react-app to reduce duplication

Extending it might also be an option.

[nickserv]

Good idea. In that case, should we edit the existing package in this repository or make a new directory in packages? I'm pretty new to Lerna and Yarn.

[paulmelnikow]

Since it's a new config rather than a fork of anything, I expect the cleanest way is to make a new directory in packages.

There must be some mechanism for publishing only certain packages, because some of the packages in this repo are not forked. I can't find it though. Maybe publish gets run manually from packages/react-scripts.

[paulmelnikow]

Can confirm b6acada is working great. 👍

Added: If anyone else wants to try it out, it's on npmjs:

  "@metabolize/react-scripts-ts": "2.16.0-metabolize.2",

[wmonk]

Hey all! Sorry i've not been involved in this, looks like some good work though. Just to double check this would allow us to have (almost) the identical rules to normal react-scripts right?

[nickserv]

Yea, my goal is to fix #333 by using CRA's existing setup with some tweaks to support TypeScript parsing. Note that the ESLint config is not configurable by the user, this is intentional as it's very minimal and meant to detect obvious bugs, but if you're willing to publish a second package I can fork their ESLint config to make it easier to use in editors and React TS projects that aren't using CRA-TS.

As far as compatibility goes, this is a breaking change in the current state because it removes all TSLint integration and brings in an ESLint config that may introduce errors (thought I would expect it to be rare). Let me know if you're OK with this, or if you want some sort of back compatibility for the TSLint integration. Right now if a user decided to migrate to this from stable CRA-TS, they would need to install and run TSLint manually to continue using it, but that shouldn't be difficult.

[wmonk]

I think this looks good. Can you update your PR to fit the new repository structure? Thanks!

[nickserv]

I can resolve the merge conflicts. I was hoping that I could still publish the forked ESLint config as a separate package so it can be used in editors conveniently (just like Displaying Lint Output in the Editor).

What would you prefer: a separate repository, a monorepo again (similar to the previous structure but with only two packages), or an orphan branch?

[InExtremaRes]

I have some doubts. With this PR are we still able to use TSLint if we want to?

In our current project we are using CRA-TS and our linter rules are different from the default ones (some are more permissive other are stricter) and we would like to keep them that way.

It would be still possible to disable the built-in ESLint and replace it with TSLint without eject?

Thank you so much!

[nickserv]

You will be able to continue using the same TSLint config if you manually install and run TSLint. TSLint will no longer be installed or run by react-scripts-ts, but tslint --project should be equivalent. ESLint cannot be disabled, but the rules are very lenient compared to TSLint's default rules, and should not cause errors unless your code is definitely broken.

[InExtremaRes]

@nickmccurdy thank you for the quick answer. So, if we want to run tslint on npm run build we will have to adjust the npm scripts? What about running it on "live" in development server (npm start)?

[nickserv]

My intention with removing TSLint was to make this more consistent with CRA itself, without generating the extra tslint.json file. Unfortunately this means a breaking change. If people disagree with this I can try to get TSLint enable optionally based on the presence of the file.

So, if we want to run tslint on npm run build we will have to adjust the npm scripts?

Correct, something like "build": "tslint --project . && react-scripts-ts build" should work.

What about running it on "live" in development server (npm start)?

You would have to eject (or use something like rewired) because I removed the TSLint config from the Webpack config. Alternatively if you don't mind running a separate script, you can use a file watcher with TSLint in another terminal.

[InExtremaRes]

@nickmccurdy Well, I understand the motivations behind this. I firmly believe that a tool like CRA, that pretends to implements a working environment the less painful and frictional as possible, should come with sane defaults that allows a "beginner developer" to enter smoothly, without a lot of unnecessary restrictions.

But IMHO a tool like that should also allows to developers to change some configurations, specially if those are about consistency, codestyle or any similar rules a team wants to enforce, under their own responsibility of course. I think there is a difference between a ready-to-go tool and a restricted one.

I don't see anyone else complaining about this, so maybe this is the preferred approach. If that is the case, maintain compatibility for TSLint could be unnecessary. We wouldn't like to eject since CRA makes a very good job for us configuring webpack, jest, etc. Using app-rewired looks reasonable to me (as I said, own responsibility) but then a template or documentation about how to optionally enable TSLint is to be very thankfully.

[nickserv]

Personally I'd rather follow CRA's approach to what is and isn't configurable as closely as possible (which is why this ESLint config isn't configurable), but I'm open to suggestions. To me it's not just about configurability, but preventing surprises for users coming from CRA. For example when I first started TSLint, I was confused about the difference between tslint.json, tsconfig.json, tsconfig.prod.json, and tsconfig.dev.json. It's also a common issue for new users to think that an error is coming from TS when it's coming from the default TSLint config.

Maybe we should have a vote on if there should be back compatability for TSLint. I'd be happy to write basic docs either way.

[nickserv]

Continued in #388.