-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Comments
Couldn't this be solved by setting your 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. |
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 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 The recommended method is 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 ( You can prove this by placing a 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 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. |
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:
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 |
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 Writing Since it's not appending (or checking if the 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 |
@methodbox It's from a comment in the discussion thread. Right before said code, the comment says this:
Sounds like it's a way to test the code in the PR. As for "permanently" modifying |
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
|
This is really needed. Trying to migrate a .js app to .ts is a hard as it is, and
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. |
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.
Also worth clarifying: even if CRA didn't overwrite There's nothing in CRA's To solve this problem, you have to modify |
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. |
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. |
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 My thought would be (as I said before) relaxing the 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). |
I'm not sure @jineshshah36 and I have the same concern as the OP actually. Let's say you quickly prototype some function: 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. |
@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 |
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 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? |
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. |
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. |
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 |
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 I haven't been able to troubleshoot it long enough to know for sure, but removing this package after starting with If/when I narrow it down, I'll post a PR if I think I can fix it and/or a bug report. |
You could replace 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. |
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:
|
@tomduncalf Would you mind sharing the |
When I'm doing a quick prototype I don't usually use one @methodbox, it has a default which seems to work okay |
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. |
It'd be be really helpful and great if this feature was implemented |
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.) |
Could you please please fix it? |
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 |
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. |
Still a problem :/ |
Fixed here #6421 |
+1 |
Why it still does not fix? Error overlay for types error in development mode is an awful idea! |
This issue can now easily be fixed. Let's imagine you don't want an overlay to show for unused variables. Do 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. |
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, bothnpm start
andnpm 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!
The text was updated successfully, but these errors were encountered: