Add TSLint rewire

[ianschmitz]

Closes #13.

This adds support for rewiring TSLint into CRA v2.

To do:

• [ ] Add documentation
• [ ] Determine dependency strategy. Currently we depend on tslint-loader, and the user is responsible for maintaining tslint plus any other config packages they may want to bring in such as tslint-react
• [ ] Decide if we should supply sensible default rules like CRA, but use tslint.json if present. For example we could detect the presence of a tslint.json, and if not present provide some default rules that align with what CRA ships with for ESLint
• [ ] Test on a larger existing TS project to determine the performance impact of tslint-loader in this configuration

TSLint in action:

[strothj]

Fantastic! Thanks for the pull request.

What do you think about removing eslint-loader if we're adding tslint?

I'll look at this in more detail on Sunday.

[ianschmitz]

Good question. I haven't dug too deep into the existing logic in this package, but at a high level it seems like it would be awesome if we could just be a "layer" on top of CRA. i.e. we just add TS support (babel preset, tslint, ...?), but don't remove support for existing stuff such as .js, flow typing, etc.

This would also allow existing projects to slowly migrate their codebase to TS as they see fit.

What do you think?

[strothj]

I just tested this on a client's project I'm working on. This particular project has about 95 components. I didn't notice any slow down and the feedback is nice.

I agree that we should proceed with not removing ESLint.

I did get quite a bit of warnings for the following:

Warning: The 'no-boolean-literal-compare' rule requires type information.
Warning: The 'no-boolean-literal-compare' rule requires type information.
Warning: The 'no-boolean-literal-compare' rule requires type information.
Warning: The 'no-boolean-literal-compare' rule requires type information.
Warning: The 'no-boolean-literal-compare' rule requires type information.
Warning: The 'no-boolean-literal-compare' rule requires type information.
Warning: The 'no-boolean-literal-compare' rule requires type information.
Warning: The 'no-boolean-literal-compare' rule requires type information.

Do you know if there's a way to disable warning for rules needing type information on the loader's side?

For reference, my tsconfig.json:

{
  "extends": [
    "tslint:latest",
    "tslint-eslint-rules",
    "tslint-config-airbnb",
    "tslint-config-prettier"
  ],
  ...
}

[ianschmitz]

I added an optional options parameter to the rewire function as you suggested earlier. It should support configuring typeCheck for tslint-loader.

For example: config = rewireTSLint(config, { typeCheck: true }) should be all you need to get rules that require type info working.

I'd be curious to see if you notice a significant difference in performance when enabled. Ultimately i think it makes sense to allow the developer to opt-in understanding the potential performance issues that still exist. We could link documentation back to https://github.com/wbuchwalter/tslint-loader/issues/76.

Regarding your question about disabling warning, i think the only way is to disable the rules requiring type info, if you decide not to enable typeCheck.

[strothj]

Thanks for the change, it seems to work.

With type checking enabled, the startup time of the project is around 3-4 minutes but with no errors from tslint.

With type checking disabled, startup time is about 15-20 seconds. Those few errors are scrolled up automatically so I don't think it's a big deal.

[strothj]

I'm currently undecided on these two points:

• Determine dependency strategy. Currently we depend on tslint-loader, and the user is responsible for maintaining tslint plus any other config packages they may want to bring in such as tslint-react
• Decide if we should supply sensible default rules like CRA, but use tslint.json if present. For example we could detect the presence of a tslint.json, and if not present provide some default rules that align with what CRA ships with for ESLint

I think including tslint-loader is fine. I don't think we need to bring in any other config packages. There doesn't seem to be a lot of version churn in either of these packages (tslint and tslint-loader). Including both I don't think would be problematic. If we want to stay on the safe side, we can just include tslint-loader.

For the second point, if we can create a rule list that mimics what react-scripts does without pulling in any dependencies for rule sets I think this would be great.

What are your thoughts on this?

[ianschmitz]

Both sound good to me. The only potential issue with us depending on tslint is due to the fact that they add rules to tslint:latest in minor versions which could be considered a breaking change. However that decision would be up to the developer to opt into the latest rule set. I don't really think it is a major concern.

It's a little tough to compare this to CRA, as they don't offer a way to customize the rules out of the box.

[ianschmitz]

For the typeCheck slowness, i think a couple things might be helpful:

• Document the issue and link to the existing GitHub issue to inform users
• Document an alternative solution for those who want rules with type info but don't want to deal with the slowness, such as avoiding the tslint rewire, and adding a separate npm script to run tslint manually just like we do with tsc

[strothj]

Hello,

I went ahead and merged in the changes as they seem fine to me. I’ll be publishing a release as soon as I track down the issue with the latest release of react-scripts.

The documentation will still need updating. I think we can leave it to the user to supply a tslint.json. We’ll explain the process in the documentation.

[codepunkt]

@strothj @ianschmitz this is great. i've been using react-scripts-ts so far, but i will try this out and then add feedback on terms of sensible linting defaults. it would be great if we could simply have sane defaults the same way cra does.