Feature idea/improvement: allow dev server and build script to run in presence of TypeScript errors

[kylebebak]

This isn't a bug report, but rather an idea to make CRA's TypeScript setup more user-friendly, and more aligned with what I understand to be the "spirit" of TypeScript.

Currently, for projects built with npx create-react-app <app> --typescript, or migrated to TypeScript as per the docs, both npm start and npm build fail in the presence of TypeScript type errors.

The dev server refuses to serve the app if non-fatal type errors are present. The build script builds it just fine, but exits with a non-zero status. In other words, npm build outputs valid code to /build that can deployed to a production server, even as it tells the user that the app "Failed to compile".

Devs migrating a moderately sized CRA app to TypeScript may literally be faced with thousands of type errors initially. None of these prevents the app from running, either locally or in production, but devs have to correct or ignore every last error (not that easy, given the limitations of @ts-ignore) before they can continue developing the app.

IMO this goes against TypeScript's philosophy of making the migration path from JS -> TS as easy as possible, and allowing devs to "progressively" add type annotations to their code.

TypeScript devs already have many options at their disposal to prevent deployment of code that has type errors, if they want to impose this constraint on themselves. CRA definitely shouldn't impose it on them.

At the very least, I think there should be an option to enable or disable this constraint. I understand that CRA's philosophy is to avoid config wherever possible. If adding an env var, e.g. TSC_COMPILE_ON_ERROR, is not desirable because it means more config, then I think the default behavior should be for CRA to function normally in the presence of type errors, while printing them as warnings to the terminal/browser console.

I opened a PR with changes that do exactly this. Looking forward to hear what people think!

[methodbox]

Couldn't this be solved by setting your tsconfig with a more passive setting (like allowing compile on errors) and doing the compile before the build?

https://www.typescriptlang.org/docs/handbook/compiler-options.html

It sounds like it's actually triggering an error because the TS compile failed (in which case you don't actually have your latest code in your build yet) but by your description, I'm not sure.

I haven't used TS w/CRA yet, but I'd imagine they are building from JS, rather than building from TS then compiling, but I could be wrong.

[kylebebak]

in which case you don't actually have your latest code in your build yet...

I definitely do.

You can write code that has type errors, save the code, and even see the updated app in the browser momentarily before the "Failed to compile" screen takes over.

And no, this isn't something that can be avoided by modifying tsconfig.json. Check out the PR if you want to, you'll see there's code, for example in react-dev-utils/WebpackDevServerUtils.js, that intercepts type errors and shows the "Failed to compile" screen if any are present.

If you change this code to not show the "Failed to compile" screen, you can still see the type errors, but the code compiles just fine (which means the "Failed to compile" message isn't really accurate anyway, but that's another, much less important issue).

[methodbox]

in which case you don't actually have your latest code in your build yet...

I definitely do.

You can write code that has type errors, save the code, and even see the updated app in the browser momentarily before the "Failed to compile" screen takes over.

And no, this isn't something that can be avoided by modifying tsconfig.json. Check out the PR if you want to, you'll see there's code, for example in react-dev-utils/WebpackDevServerUtils.js, that intercepts type errors and shows the "Failed to compile" screen if any are present.

If you change this code to not show the "Failed to compile" screen, you can still see the type errors, but the code compiles just fine (which means the "Failed to compile" message isn't really accurate anyway, but that's another, much less important issue).

I'm going to try this myself, just because I'm curious, but have you actually tried this by creating a new app using the command in the docs, instead of npx create-react-app cra-typescript --typescript?

The recommended method is npx create-react-app my-app --typescript.

https://facebook.github.io/create-react-app/docs/adding-typescript#docsNav

Edit - I tested this and it does in fact throw a proper error in dev mode (npm start) - as I would expect it to since there is a TS error; ultimately CRA is referencing the tsconfig / the compiler for this error, then deciding to quit.

You can prove this by placing a // @ts-ignore before the offending line. For example, I used this to test:

import React from "react";
import logo from "./logo.svg";
import "./App.css";
//  @ts-ignore
const test: string = 10;

const App: React.FC = () => {
  return (
    <div className="App">
      ...
    </div>
  );
};

export default App;

In both the case of npm start and npm run build it worked fine. This indicates the compiler is being referenced for the error.

Edit - I see that your PR code makes this an optional boolean, which I like. Personally I'd rather it did not ignore the error, unless I had acknowledged it and chosen to do so as the potential for side effects would not necessarily be clear.

There may be non-fatal errors that still introduce bugs, even security issues.

[jineshshah36]

I would like to express my support for some more user-friendly way to deal with ts compilation errors. We have a very large typescript codebase so the experience we have right now is:

1. cra compiles js and app loads
2. 10-15 seconds later, the screen gets covered by a non-dismissible overlay that shows a type error that could be as trivial as "unused import"
3. We fix the issue and have to wait for recompilation.

In addition to that scenario, we also get the issue of type errors that have already been fixed appearing as errors because the compiler is behind.

Any method to either allow the overlay to be dismissible or disable compilation failure on type error would be really great

[methodbox]

I agree with most of this, in theory, but I question this approach (from the PR):

//  package.json
{

    "start": "echo 'REACT_APP_ENV=dev\nTSC_COMPILE_ON_ERROR=true' > .env && react-scripts start",
    "build": "echo 'REACT_APP_ENV=prod\nTSC_COMPILE_ON_ERROR=true' > .env && react-scripts build",
    "build-stg": "echo 'REACT_APP_ENV=stg\nTSC_COMPILE_ON_ERROR=true'  > .env && react-scripts build",

}

Your command relies on bash (we can't assume someone won't be using sh or csh and echo doesn't always have the same behavior in different shells) and it also rewrites the .env file every time that command is run.

Writing echo > anything will overwrite the entire file with just anything, so each time you're rewriting the entire file, which means destroying what is there.

Since it's not appending (or checking if the .env parameter already exists), and because there is always a possibility of additional config in a user's environment being overwritten, I feel like that's a non-starter as its destructive and unpredictable.

Generally, that type of a command shouldn't be associated with a startup script. If it's not your intention to include that as a prereq to the PR, maybe that should be clarified in the PR.

I'd like to see an approach that's as simple as npm start --ts-ignore-errors that doesn't require modifications to the .env to run on a per-case basis, and maybe allows a modification to .env to make "permanent". A simple flag argument shouldn't require modifying the .env, much less a destructive file write.

[kylebebak]

@methodbox
The code you put in your comment above is obviously not "from the PR".

It's from a comment in the discussion thread. Right before said code, the comment says this:

To test it out, you can just create a bare TypeScript app with npx create-react-app cra-typescript --typescript, then change the react-scripts dep as follows.

Sounds like it's a way to test the code in the PR.

As for "permanently" modifying .env, you could always create your own .env file, add whatever you want to it, then run npm start.

[methodbox]

@kylebebak

There’s no need to be a jerk when also arguing semantics.

I was clearly referring to the “PR” including the comments.

My suggestion is that if you intend to make a change that requires an edit to the .env then:

1. This particular case seems like it would be a good place for a command line arg, since many will want the option to bypass as needed, but not necessarily configure an environment variable for this.

2. Maybe give that example of editing the .env in your PR COMMENTS (just want to be clear here) instead of the current PR COMMENTS which, while an example of how to test this, still include bad and possibly destructive practice that might be terrible advice for someone wanting to test it out who doesn’t realize the repercussions of overwriting a potentially vital .env config.

[cristianfraser]

This is really needed. Trying to migrate a .js app to .ts is a hard as it is, and cra overwrites compilerOptions is tsconfig.json if you try to edit them, so there is no solution going that way.

Personally I'd rather it did not ignore the error, unless I had acknowledged it and chosen to do so

That's what the option would do. We are acknowledging the errors are there. Our editors still show them, the console output will still show them, but we will be able to develop our apps. Developing is not a road without errors, forcing the overlay and preventing the build without an option to not do so is not helpful.

[kylebebak]

@cristianfraser

Just to clarify, the PR linked to in this issue doesn't ignore any errors. It does what you suggest -- it prints the errors to the terminal and to the browser console, but doesn't prevent the dev server from serving the app in the presence of TypeScript errors.

and cra overwrites compilerOptions is tsconfig.json if you try to edit them, so there is no solution going that way.

Also worth clarifying: even if CRA didn't overwrite compilerOptions, you wouldn't be able to solve this problem by writing your own.

There's nothing in CRA's compilerOptions that prevents TypeScript from compiling your TS files to JS. It compiles the files just fine, it just refuses to serve them. If you make a change to your source code and save it, you can even see the updated app in the browser momentarily before the "Failed to compile" screen takes over.

To solve this problem, you have to modify react-dev-utils/WebpackDevServerUtils.js, which is what is done in the PR.

[MattMorrisDev]

2. 10-15 seconds later, the screen gets covered by a non-dismissible overlay that shows a type error that could be as trivial as "unused import"

This is the annoying part. You might want 'noImplicitAny' or 'noUnusedImports' to be strictly enforced during prod builds or code reviews, but having it show the error overlay during development with no way to dismiss it despite it being perfectly valid emitted javascript is frustrating.

It seems like the error overlay used to be dismissible at some point, or maybe its just not dismissible for typescript errors, but its a pain point either way.

The errors are already being printed in the terminal and the browser console, so having the option to dismiss or opt out of the overlay errors would be awesome.

[jineshshah36]

@methodbox

It would be great to get some insight from a contributor on what the current thought process is for not making errors dismissible and what would be required to change that.

[methodbox]

@jineshshah36

I’m a contributor but I’m certainly not a maintainer or owner of this package, but I’ll give my two cents, though I don’t think you’ll like it ;).

I’m wondering how exactly the process would go, just in matter of order of processing, to build the app but also have type errors.

The JS that is used for the build is first compiled from the associated TS files.

If you’re just writing TS (not inside a CRA project) and have the watch TS build task running, essentially what happens in CRA, you don’t get any JS files if your TS fails type checking unless you’ve configured your tsconfig in such a way to allow it. Even then, you don’t always get it to compile depending on the error.

My thought would be (as I said before) relaxing the tsconfig, and on the CRA side allowing certain errors, but the tsc or babel compilers aren’t going to care what CRA wants; they are going to fail to compile in instances where they would fail otherwise, unless excepted by tsconfig or some argument on the CLI.

CRA ultimately fails because files required to make the build, the JS not the TS fail to be generated, which is up to the compiler.

I’m still not clear why the OP would not be able to see TS errors before compile, since they are linted in the TS already; it would be obvious this is going to fail to compile.

The argument that migrating a project to TS is going to result in, basically, a lot of work fixing type errors begs the question why you would want to build the app from that instance vs the JS instance when you haven’t fixed any of the type issues?

In my experience, moving an app from JS to TS, the most common occurrence of type errors stems from two things: you haven’t installed the applicable @types package(s) and/or you haven’t imported the necessary types from those packages (less common as it’s usually detected).

I just migrated 8 screens and an API from RN to TS and these are the only issues I faced beyond some ambiguity which is (and shouldn’t be) allowed in JS and not in TS (like using a global which isn’t locally defined - think “ref” on the instance).

[MattMorrisDev]

I'm not sure @jineshshah36 and I have the same concern as the OP actually.

Let's say you quickly prototype some function: const func = (x) => doThing(x) and then save. After a few seconds, CRA will have emitted the new perfectly valid javascript bundles. Now you're trying to test out the new function in the browser after your live-reload kicks in. Imagine you're clicking around on the page, and you're about to get to the point where you do whatever is necessary to trigger your function - you've already invested some time and effort. Suddenly the error overlay appears out of nowhere to complain that x in your function is implicitly typed as any (because the async type-checking process has just finished). The critical issue here is that there is no way to dismiss the error overlay.

The error overlay says "This error occurred during the build time and cannot be dismissed.", but it didn't occur during build time (since babel just strips the types before compiling/handing off to webpack - it actually happens during the async type checking), and it clearly can be dismissed (you can delete the overlay iframe using devtools, and the app would work fine in this case).

It's easy to say "just give x a type", but there are plenty of situations where its just not that simple (or you might come back to add types later after sanity testing your prototype).

IMO the overlay should only be non-dismissible if the actual babel compilation / webpack bundling process fails.