Skip to content
This repository has been archived by the owner. It is now read-only.

TSLint errors cause compile failure (can't use console.log or debugger statements in development) #329

Closed
BernieSumption opened this issue May 20, 2018 · 12 comments

Comments

@BernieSumption
Copy link

@BernieSumption BernieSumption commented May 20, 2018

Another similar issue 171 deals with Typescript errors caused by semantic checks like noUnusedGlobals. This issue refers to the fact that TSLint errors cause compile errors.

The difference between this and the above issue is that it is possible to argue that it is good for developers to be forced to adhere to tsc's semantic checks as they develop, and indeed in the above linked issue many people do argue this.

It is much harder to argue that developers should be forbidden from using console.log or debugger statements during development. How else are we supposed to debug our applications?

Current workarounds:

  1. Disable the TSLint rule (now console.log statements may find their way into release code)
  2. Eject the app and configure it to not make TSLint errors fatal

Is this a bug report?

Yes

Can you also reproduce the problem with npm 4.x?

N/a

Which terms did you search for in User Guide?

console.log, debugger, tslint

Environment

  1. npm ls react-scripts-ts (if you haven’t ejected): react-scripts-ts@2.16.0
  2. node -v: v8.10.0
  3. npm -v: n/a
  4. yarn --version (if you use Yarn): 1.6.0

Then, specify:

  1. Operating system: Mac
  2. Browser and version (if relevant): n/a

Steps to Reproduce

(Write your steps here:)

  1. create-react-app my-app --scripts-version=react-scripts-ts
  2. Edit src/App.tsx' to add a debugger` statement
  3. yarn start

Expected Behavior

Debugger pauses on debugger statement

Actual Behavior

Failed to compile
/Users/bernie/Desktop/untitled folder/demo/src/App.tsx
(8,5): Use of debugger statements is forbidden

image

Reproducible Demo

@ethanneff

This comment has been minimized.

Copy link

@ethanneff ethanneff commented May 21, 2018

maybe related to space in your path? untitled folder

@BernieSumption

This comment has been minimized.

Copy link
Author

@BernieSumption BernieSumption commented May 22, 2018

@ethanneff I don't think it's that kind of bug - the build system is behaving as configured. It's just configured to treat linter warnings as fatal.

For my current project I have decided to ditch create-react-app and build my own webpack config, but if the maintainer(s) agree that this is an issue worth fixing, and give me some pointers as the the appropriate way to fix it, I'm happy to put together a PR.

@DorianGrey

This comment has been minimized.

Copy link
Collaborator

@DorianGrey DorianGrey commented May 24, 2018

This is not a bug, but part of the default set of tslint rules (no-debugger).

Strictly speaking - it's intended behavior. Both disabling the rule and lowering its level will result in a high risk such commands end up in a production build. The only reasonable alternative would be to use a different tslint configuration for build mode which enforces more strict rules and/or error levels than the one for development.

@BernieSumption

This comment has been minimized.

Copy link
Author

@BernieSumption BernieSumption commented May 24, 2018

In that case, the intended behaviour diverges from create-react-app (which displays no warnings or errors at all for console.log and debugger rules), previous versions of react-scripts-ts (which used ts-loader so no tslint errors were fatal) and ForkTSChecker (which recommends setting defaultSeverity to "warning" in tslint.json).

console and debugger statements are fundamentally different from other lint errors because you can't take the view that developers should just fix them immediately: they legitimately need to be in the source code for a while in order to trigger useful browser features.

In reading other threads on this topic I've discovered a combination of features that, had I known about them last week, would probably have stopped me from writing my own webpack config (I'm not going back now, I like Babel 7 too much :)

  1. set defaultSeverity to "warning" in tslint.json
  2. change the build script in package.json to "build": "CI=true react-scripts-ts build"

If this were my tool, I'd make that the default behaviour, but it's not my tool :)

@sw-yx

This comment has been minimized.

Copy link

@sw-yx sw-yx commented May 31, 2018

serious question: why not fork this project and make other choices? what other key DX decisions has CRATS made that could deserve a nice number 2 typescript react-script alternative?

@markerikson

This comment has been minimized.

Copy link

@markerikson markerikson commented May 31, 2018

Agreed. I just tried CRA-TS myself for the first time a few days ago, while helping a newbie get set up. He had some bits of TS code already written in another project that had a self-written Webpack+TS config, and I wanted to make it easy for him to switch over to a "proper" build setup so he wouldn't have to worry about any of that.

It was an awful experience.

I'm a very experienced React dev, but have never used TS. I spent more time trying to fight the many lint errors that CRA-TS was throwing out than I did actual compile problems. In addition, not only is every lint rule set to be an error (as far as I can tell), but the rules are excessively strict. No console logs? No arrow functions in render? I understand why some people might want to turn those on, but for a project based on Create-React-App, this is a horrible choice.

Create-React-App's linting philosophy is "tell the user when they're doing something that will cause actual bugs", like using undefined variables. Meanwhile, "no arrows in render" is not only a heavily opinionated style choice, 95% of the time it's not even a meaningful performance difference. This is not a good way to ease in newer devs.

I realize that as a maintainer, it's up to you to decide what the config settings should be, and if the community doesn't like it, they can fork things. But, as the recommended "Create-React-App with TypeScript" repo, I'd seriously encourage you to keep the lint rules light, simple, non-errors, and in parallel with the choices made by CRA itself.

@wmonk

This comment has been minimized.

Copy link
Owner

@wmonk wmonk commented May 31, 2018

I'm happy to relax these rules to fit the original philosophy of CRA. When I originally created this module I copied over a bunch of restrictive rules which we were using at work. This was then changed in #281 to use some recommended presets. My intention was never to cause a bad experience to new or experienced devs trying to use TypeScript, and the strictness are just defaults that come from those presets.

If someone would like to take the lead on porting a similar ruleset from the eslint config or CRA - https://github.com/facebook/create-react-app/blob/next/packages/eslint-config-react-app/index.js - then I'm happy to accept a PR for it.

@InExtremaRes

This comment has been minimized.

Copy link

@InExtremaRes InExtremaRes commented Jun 9, 2018

I agree! For me a rule as no-console is a very good one for production code but in fact it is not so handy while developing. As other commented we can disable tslint for a line and put a console.log, but then you can just forget to delete it.

The default IMHO should be a warning in npm start but an error in npm run build.

@Nasicus

This comment has been minimized.

Copy link

@Nasicus Nasicus commented Aug 8, 2018

@InExtremaRes or anyone else:
Did you find a work around for this? I find it very annoying.
What I have done so far is to add "defaultSeverity": "warning" to tslint.json (so now the dev mode works as desired), however when doing a prod build (npm run build respectively react-scripts-ts build) it now also shows the lint errors only as warnings instead of errors.

Edit:
Found a solution thanks to this post:
#133 (comment)
Basically I created a tslint.prod.json which looks like this:

{
  "extends": ["./tslint.json"],
  "defaultSeverity": "error"
}

Then I added in the package.json the following to the build task:
"build": "tslint --project . --config tslint.prod.json && react-scripts-ts build"

jansule added a commit to jansule/geostyler that referenced this issue Aug 13, 2018
Now two tslint configs exist. One for development, one for production. In development, the defaultSeverity is set to "warning". Thus, compiling does not fail when breaking a rule. In production defaultSeverity is set to "error", resulting in failing compilation.

When running `npm run build` tests run before building.

Solution for config setup taken from
wmonk/create-react-app-typescript#329 (comment)
wmonk/create-react-app-typescript#133 (comment)
jansule added a commit to jansule/geostyler that referenced this issue Aug 16, 2018
Now two tslint configs exist. One for development, one for production. In development, the defaultSeverity is set to "warning". Thus, compiling does not fail when breaking a rule. In production defaultSeverity is set to "error", resulting in failing compilation.

When running `npm run build` tests run before building.

Solution for config setup taken from
wmonk/create-react-app-typescript#329 (comment)
wmonk/create-react-app-typescript#133 (comment)
@fgm

This comment has been minimized.

Copy link

@fgm fgm commented Aug 30, 2018

Case in point I have: a logging package which has one output class using the console, while the others go to files, syslog, etc. That specific class needs to access the console because that's just its job.

@slikts

This comment has been minimized.

Copy link

@slikts slikts commented Sep 4, 2018

It's very unhelpful to fail compilation for things like unused names during development. I've tried adding "defaultSeverity": "error", to tslint config, but that only seems to affect my editor, and the build still fails.

@wmonk

This comment has been minimized.

Copy link
Owner

@wmonk wmonk commented Sep 4, 2018

Please follow #388

@wmonk wmonk closed this Sep 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
10 participants
You can’t perform that action at this time.