Skip to content

Add support for TSLint #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ianschmitz opened this issue Jun 10, 2018 · 4 comments
Closed

Add support for TSLint #13

ianschmitz opened this issue Jun 10, 2018 · 4 comments

Comments

@ianschmitz
Copy link
Contributor

ianschmitz commented Jun 10, 2018

I wanted to start the conversation to see if there's interest in supporting TSLint within this package. With the addition of TSLint I think this package would be a great solution for the majority of users whom want to use CRA, but wish to have TypeScript support, without having to use a forked version of CRA such as https://github.com/wmonk/create-react-app-typescript/.

Alternatively I could publish the TSLint support as separate package that would work nicely with this one.

I'd be happy to help contribute if there's interest!

@strothj
Copy link
Owner

strothj commented Jun 10, 2018

Thanks for the kind words! I've enjoyed using this project in a lot of the work I do.

I like your idea, I think something like this would be good:

const {
  rewireWebpack: rewireTypescript,
  rewireJest: rewireTypescriptJest,
  rewireTSLint
} = require("react-app-rewire-typescript-babel-preset");

module.exports = {
  webpack: function(config, env) {
    let rewiredConfig = config;

    rewiredConfig = rewireTypescript(rewiredConfig);
    rewiredConfig = rewireTSLint(rewiredConfig, ...); // accept an optional second options parameter here to pass along to tslint-loader

    return rewiredConfig;
  },
  jest: function(config) {
    return rewireTypescriptJest(config);
  }
};

The rewireTSLint should remove react-scripts's ESLint loader and replace it. Also, either another example added to the monorepo or the current example extended to use it.

What do you think? If you get this working, I'll be happy to merge it in!

@ianschmitz
Copy link
Contributor Author

ianschmitz commented Jun 10, 2018

I was thinking along those lines! We have at least 2 good options here:

Use tslint-loader as you suggested above

Pros:

  • The linting experience should be very familiar to anyone used to CRA's way of doing things. i.e. npm start shows lint errors as warnings in the terminal/browser console. npm run build causes lint errors to fail the build if CI=true

Cons:

  • Linting isn't integrated with the tsc type checking
  • There have been reports of tslint-loader slowing down the build significantly (i haven't used the loader personally so i can't weigh in on this). See typeCheck is too slow wbuchwalter/tslint-loader#76. I use the TS fork of CRA on a few different projects at work, and I always notice the speed difference when i switch to a plain JS project -- the TS flavor is noticeably slower during npm start or npm test. The fork doesn't use tslint-loader to my knowledge, but i feel that speed is important!

Use tslint-language-service

Pros:

  • There should be minimal/no work needed from this plugin. Users would specify the plugin in their tsconfig.json and use npm run type-check which would both type check and lint their code
  • Should be more efficient than tslint-loader. This seems to be the way the VSCode TSLint plugin team is moving -- here is the preview version that uses the language service: https://github.com/Microsoft/vscode-ts-tslint

Cons:

  • The linting experience has diverged from what vanilla CRA offers. Users are presented with the lint errors much later in the development cycle potentially (depending how often they run npm run type-check). However this is already true for the TypeScript type checking, so I don't know that this is a big issue
  • The repo doesn't seem to be very active: https://github.com/angelozerr/tslint-language-service/issues

@strothj
Copy link
Owner

strothj commented Jun 10, 2018

My current work flow is this:

  • I use the normal TSLint extension for VSCode which shows me the errors in my editor.
  • Linting errors don't show up in the browser, unless it's one that is caught by CRA's ESLint (no-used-vars, etc...)
  • I run TSLint command line any time I need a list of type errors (maybe during a refactor, etc). I run the command yarn type-check, which is "type-check": "tsc", (my tsconfig.json has the noemit option enabled).
  • CI runs scripts: lint, type-check

For the tslint-language-service plugin:
I've used this in the past. Since we don't actually use the TypeScript compiler (we just strip the typing syntax using the Babel plugin), this plugin would not normally get invoked. The user would need to invoke the typescript compiler using a package.json script.

I think the value we would be adding with this would be integration with the Webpack error page. If the user isn't interested in having the errors displayed in the browser during development, they can instead invoke TSLint through a script or integrate tslint into a tsc step using the language service.

If you pass along the tslint-loader options, the user will have the option to use type checking mode or not depending on their performance requirements. I have a plugin for Storybook that parses TypeScript information for components and it does cause a significant slow down (maybe my implementation is bad...).

I think the only scenario that requires changes on the part of the rewire is the one where we add tslint-loader support. Because we'll be offering it as a separate rewire, the behavior will be opt-in.

@strothj strothj closed this as completed Jun 10, 2018
@strothj strothj reopened this Jun 10, 2018
@strothj
Copy link
Owner

strothj commented Jun 10, 2018

Pardon the close/reopen, fat fingered on mobile. :p

@ianschmitz ianschmitz mentioned this issue Jun 16, 2018
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants