Skip to content
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

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

Closed
kylebebak opened this issue Apr 26, 2019 · 34 comments · Fixed by #6931

Comments

@kylebebak
Copy link
Contributor

kylebebak commented Apr 26, 2019

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
Copy link
Contributor

methodbox commented Apr 27, 2019

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
Copy link
Contributor Author

kylebebak commented Apr 27, 2019

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
Copy link
Contributor

methodbox commented Apr 27, 2019

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
Copy link

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
Copy link
Contributor

methodbox commented May 1, 2019

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
Copy link
Contributor Author

kylebebak commented May 2, 2019

@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
Copy link
Contributor

methodbox commented May 2, 2019

@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
Copy link

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
Copy link
Contributor Author

@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
Copy link

MattMorrisDev commented May 21, 2019

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
Copy link

@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
Copy link
Contributor

methodbox commented May 22, 2019

@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
Copy link

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.

@jineshshah36
Copy link

@methodbox I appreciate the thorough response! And yes, @MattMorrisDev is correct that is essentially the gist of the problem. We literally just want an "x" in the corner. If you think it makes more sense to open a separate issue and explain that, I'm more then happy to do that

@methodbox
Copy link
Contributor

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.

Maybe the question should be (or the solution): should the in-browser errors even contain non-breaking TS compile errors at all?

I.E. if the page otherwise loads and the build is complete, should those errors ever bubble up to the on-page error, or should they be restricted to linting/problems log?

I know this is kind of what the OP is getting at, but I mean it really is a fundamental question of how CRA is handling errors in general; eslint errors that aren't breaking don't necessarily cause this kind of an issue.

On the other hand, the any type warning should be seen in-editor before you ever build. If you truly want to ignore this, isn't that the responsibility of your tsconfig?

I'm not sure exact what kind of error handling system CRA is using to push those errors to the browser, but I'd bet it's more all-encompassing than pick-and-chose, so the OP's approach of making it optional seems reasonable, assuming it doesn't break anything else.

I wouldn't use it (I'd rather see the error) but then I don't think I've experienced exactly what you guys are describing - are you not seeing the error in the editor?

@MattMorrisDev
Copy link

(I'd rather see the error) but then I don't think I've experienced exactly what you guys are describing - are you not seeing the error in the editor?

I can see the typescript errors in my editor. They also show in the terminal and the browser console. There's three places already displaying the error, which is why it would be nice to make the fourth one dismissible (or avoidable 😀) since it blocks usage of the page.

I added some css to my project (for dev builds only) to hide the error iframe as a short term solution, but it seems like the dev experience for TS+CRA could be polished slightly in regards to the error overlay.

@jineshshah36
Copy link

We are seeing the error in the editor, but the problem is specifically that even after we have fixed the error, we get an overlay because the ts compilation lags behind.

@zsiglin
Copy link

zsiglin commented May 28, 2019

This has been an extreme pain point for us as well. We're trying to add TypeScript support to a medium sized project and I'm unclear on a path forward for migration. The project builds and works normally with standard JS, but as expected there are a plethora of errors that need to be addressed when TypeScript is introduced. We do not have the time or resources to go through each and every one of these immediately. As it stands I haven't discovered a way to get the project to build without fixing everything.

I would expect there'd be some ability to log these "errors" as warnings and continue on with the build process so that code can be migrated incrementally as time allows. Have others found a suitable approach? The cleanest thing I have come up with is to use a mix of .js[x] and .ts[x] files in the interim rather than renaming everything all in one go.

@methodbox
Copy link
Contributor

Guys, I've been testing this all week and I think that it's actually a bug.

While the OP's PR certainly has merit, I think there is also a bug, maybe related to eslint-config-react-app - this seems to be causing some major headaches as it's fighting with VS Code ( I think) and the TS Lint extension.

I haven't been able to troubleshoot it long enough to know for sure, but removing this package after starting with npm start seems to alleviate the issue, however it won't start again after as CRA relies on this and checks for it at start.

If/when I narrow it down, I'll post a PR if I think I can fix it and/or a bug report.

@kylebebak
Copy link
Contributor Author

kylebebak commented May 28, 2019

@zsiglin

I would expect there'd be some ability to log these "errors" as warnings and continue on with the build process so that code can be migrated incrementally as time allows. Have others found a suitable approach?

You could replace "react-scripts": "<VERSION>", with "react-scripts": "fortana-co/react-scripts#master", in package.json.

You would be using the version of react-scripts from my PR, which prints the TS errors to the terminal and the browser console, but doesn't prevent the app from loading.

This is what I'm doing with my projects until the PR or something similar gets merged. And yeah, I agree, this is a major pain.

@tomduncalf
Copy link

tomduncalf commented Jun 17, 2019

This is a major pain point for me as well. My main use-case for CRA is quickly trying out ideas/hacking something together. I still want Typescript in this situation, but I'm not worried about most errors to start with – I'll gradually fix it up as I go. As it is, CRA is unusable for this, which is a shame.

Instead, I recommend using Parcel bundler, which actually isn't that much more work and could easily be wrapped in a script:

mkdir my-project
cd my-project
yarn init -y
yarn add parcel typescript react react-dom @types/react @types/react-dom
mkdir public
  • edit public/index.html so it has, at a minimum, <script src='../src/index.tsx'></script>
  • edit src/index.tsx so it renders whatever you want
  • add a package.json script to run parcel public/index.html (or run it with node_modules/.bin/parcel public/index.html)

@methodbox
Copy link
Contributor

@tomduncalf Would you mind sharing the tsconfig you’re using along with Parcel?

@tomduncalf
Copy link

When I'm doing a quick prototype I don't usually use one @methodbox, it has a default which seems to work okay

@Yankovsky
Copy link

Yankovsky commented Jul 9, 2019

Currently it is very difficult and frustrating to develop CRA app with typescript because of this problem. I think that OP approach with adding optional env var is just fine. I even think that by default TS compilation errors should be warnings.

@ArtyomResh
Copy link

It'd be be really helpful and great if this feature was implemented

@vislogurov
Copy link

Guys, how is it going? I'm looking forward to use CRA without any hacking. The inability to start compiling forces me to write my own unmaintainable hacks every time again and again. I throw myself on your mercy.)

@lise-yy
Copy link

lise-yy commented Jul 9, 2019

Could you please please fix it?

@Windrushfarer
Copy link

Our team now really need way to start server if we have TypeScript errors. This improvement will help us. I think way using some argument flag will be useful, something like --ignore-typescript-errors

@maksim-kononov-csssr
Copy link

I encountered a similar problem. I have some JS codebase I want translate to TS, but I cant do this step by step - I have to translate WHOLE component before I can see results on my screen. I think it is nice to have an option to ignore TS errors on translate period.

@socksrust
Copy link

Still a problem :/

@sibelius
Copy link

Fixed here #6421

@jischein
Copy link

jischein commented Aug 2, 2019

+1

@artalar
Copy link

artalar commented Aug 24, 2019

Why it still does not fix? Error overlay for types error in development mode is an awful idea!

@jineshshah36
Copy link

This issue can now easily be fixed. Let's imagine you don't want an overlay to show for unused variables. Do the following:

  1. Set noUnusedLocals to false in tsconfig.json
  2. Create a custom eslint config that looks like the following:
{
  "root": true,
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "project": "./tsconfig.json",
    "sourceType": "module",
    "jsx": true,
    "ecmaFeatures": { "jsx": true }
  },
  "extends": ["react-app"],
  "rules": {
    "no-unused-vars": ["warn"],
  },
  "overrides": [
    {
      "files": ["**/*.ts?(x)"],
      "plugins": ["@typescript-eslint"],
      "extends": [
        "plugin:@typescript-eslint/eslint-recommended",
        "plugin:@typescript-eslint/recommended",
      ],
      "rules": {
        "@typescript-eslint/no-unused-vars": [
          "warn",
          {
            "ignoreRestSiblings": true,
            "varsIgnorePattern": "^_",
            "argsIgnorePattern": "^_",
            "caughtErrors": "all"
          }
        ]
      }
    }
  ]
}

You will no longer get an overlay but will get a warning in the console. You can also set it to off if you don't want to see the console warning.

@lock lock bot locked and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.