Show warnings instead of compiler errors

[szmarci]

I started a project now with create-react-app-typescript and everytime I make a mistake (according to tslint rules) it's not compiling until I fix it. It is strange, because I made a project couple of months ago with this tool and it showed the errors, but it did compile my code. I didn't change anything in the config files. How can I switch the beaviour back?

[DorianGrey]

Indeed - the plugin we're using for tslint refers to the severity defined configuration file (i.e. tslint.json). If there is no severity defined, everything will fire up on error level.

It should be possible to use the warning level by adding "defaultSeverity": "warning" to the tslint.json in the generated project's root directory. Can you give this a try?

[szmarci]

Thanks for stopping by. I tried adding this to tslint.json but it still fails to compile. (I checked the older project's tslint.json and there are no "defaultSeverity": "warning" in there either)

[szmarci]

Sorry, it works, I put it under rules the first time.

[DorianGrey]

(I checked the older project's tslint.json and there are no "defaultSeverity": "warning" in there either)

Just a note about this: Earlier versions used tslint-loader, which does not interrupt the build on linter failures by default. Current versions use fork-ts-checker-webpack-plugin for linting and typechecking in parallel, which counters performance penalties by sharing the ts.Program instance required for both. It just picks up the linter configuration, and thus only takes care of the severity setting if it's present and falls back to the default one otherwise - which is error.

[szmarci]

@DorianGrey thanks for the clarification.

[GeeWee]

Thoughts on changing the severity generated in the boilerplate file? I think it makes a lot of sense to have them be warnings instead of errors.

[DorianGrey]

Personally, I prefer linter errors to be quite aggressive, so that I'm forced to deal with them as soon as possible - even if that might be a bit tedious.

However, it might make sense to reduce these messages down to warnings at least for development, but not for build mode. That'd be possible by providing two tslint config files, where the one for build simply inherits the one for development and just overrides the defaultSeverity configuration. Still thinking about this.

[GeeWee]

I'm just the opposite, especially considering that many linting errors can be fixed automatically - my workflow is leave the errors, run the entire project through autofix and then fix what's left. One size fits all workflow is probably not possible here I'd guess.

[szmarci]

I'm on team warning with this. But it is definitely down to personal preference. I like to quickly see the result of my code and then later fix them all in one run.

[caghand]

@DorianGrey, what you are planning is easily achievable using the CI environment variable. Please see here: https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template/README.md#continuous-integration

So apparently, in the upstream create-react-app, the usual practice is to treat linter issues as warnings during development, but as breaking errors during build. To be compatible with the upstream package, you might want to add defaultSeverity: warning in the boilerplate file. I don't have strong opinions either way, but I have seen that this usual practice works pretty well in our team.

Thanks!

-Caghan

[DorianGrey]

@caghand I'm not sure if this CI variable will work in our case, since a different linter and a different way to execute it are used. IIRC, neither of them takes care of this variable. Also, overriding the default severity via config file instead of plugin config makes this easier to reason about (at least I hope it will).

[caghand]

I can't say I understand all the details about create-react-app, but I am sure the CI variable works with the latest create-react-app-typescript. I've tested it. :) I mean, I've set defaultSeverity: warning in tslint.config, and I was able to make the warnings break the build by setting CI=true.

-Caghan

[DorianGrey]

OK, found it: https://github.com/wmonk/create-react-app-typescript/blob/7c0032bf6db21fd236fe6d7bc1d4b419ff946420/packages/react-scripts/scripts/build.js#L129-L141 That definitely requires some easier to find documentation, since this behavior is not only suitable for CI builds ...

[mattmazzola]

This might be relevant here: https://github.com/facebook/create-react-app/issues/3925

I agree that in development they should be warnings, and by default when CI is set warnings are treated as errors to align with the original CRA behavior; however since this project adds tslint which diverges, I would propose allowing another option to not treat the warnings as errors even when CI is enabled.

This could be opened as separate issue for independent control of jest single run and tslint warnings, but this issue seems related.

[jonathaningram]

I've held off on commenting for a few days, but I think I have this same issue:

Compiling...
/Users/me/my-app/www/ui/node_modules/fork-ts-checker-webpack-plugin/lib/service.js:22
        throw error;
        ^

FatalError: 
Invalid source file: /Users/me/my-app/www/ui/src/twirp.ts. Ensure that the files supplied to lint have a .ts, .tsx, .d.ts, .js or .jsx extension.

    at new FatalError (/Users/me/my-app/www/ui/node_modules/tslint/lib/error.js:27:28)
    at Linter.getSourceFile (/Users/me/my-app/www/ui/node_modules/tslint/lib/linter.js:222:23)
    at Linter.lint (/Users/me/my-app/www/ui/node_modules/tslint/lib/linter.js:96:31)
    at /Users/me/my-app/www/ui/node_modules/fork-ts-checker-webpack-plugin/lib/IncrementalChecker.js:142:30
    at WorkSet.forEach (/Users/me/my-app/www/ui/node_modules/fork-ts-checker-webpack-plugin/lib/WorkSet.js:17:13)
    at IncrementalChecker.getLints (/Users/me/my-app/www/ui/node_modules/fork-ts-checker-webpack-plugin/lib/IncrementalChecker.js:139:17)
    at run (/Users/me/my-app/www/ui/node_modules/fork-ts-checker-webpack-plugin/lib/service.js:15:29)
    at process.<anonymous> (/Users/me/my-app/www/ui/node_modules/fork-ts-checker-webpack-plugin/lib/service.js:38:5)
    at process.emit (events.js:160:13)
    at emit (internal/child_process.js:790:12)

After the first load, this happens every time I save a file and the app tries to reload.

It doesn't appear to be related to linting since there are no linting errors:

$ which tslint
node_modules/.bin/tslint
$ tslint --version
5.9.1
$ tslint -c tslint.json "src/**/*.ts{,x}"
$ echo $?
0

Unless cra-typescript is running different settings for linting.

Just in case, I've tried changing the severity in tslint.json:

{
  "extends": ["tslint-react"],
  "defaultSeverity": "warning"
}

But that's the same issue.

Here's my package versions:

{
  "dependencies": {
    "@types/jest": "^22.1.1",
    "@types/node": "^9.4.0",
    "@types/react": "^16.0.35",
    "@types/react-dom": "^16.0.3",
    "@types/react-helmet": "^5.0.3",
    "@types/react-router-dom": "^4.2.3",
    "jest-styled-components": "^4.10.0",
    "loadable-components": "^0.4.0",
    "prettier": "^1.10.2",
    "react": "^16.2.0",
    "react-dom": "^16.2.0",
    "react-helmet": "^5.2.0",
    "react-router-dom": "^4.2.2",
    "react-scripts-ts": "2.13.0",
    "react-snap": "^1.11.1",
    "source-map-explorer": "^1.5.0",
    "styled-components": "^3.1.4",
    "ts-jest": "^22.0.2",
    "tslint": "^5.9.1",
    "tslint-react": "^3.4.0",
    "typescript": "^2.6.2",
    "typescript-styled-plugin": "^0.4.0",
    "whatwg-fetch": "^2.0.3"
  }
}

Edit: I've tried 2.8.0 and that version does not have this issue.

Any ideas?

[GeeWee]

Definitely don't think that's the same issue. I would suggest opening a new one :)

[DonaldMackay]

"defaultSeverity": "warning" will only work for tslint rules. If tsconfig.json has "noUnusedLocals": true then webpack will return error to UI regardless of tslint settings.

[mbrowne]

In case anyone is interested, here's what I came up with to only throw a warning rather than an error (in development only) for unused locals: https://github.com/wmonk/create-react-app-typescript/issues/218#issuecomment-374739265