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

Adds TSC_COMPILE_ON_ERROR env var... #6931

Merged
merged 7 commits into from
Oct 1, 2019
Merged

Conversation

kylebebak
Copy link
Contributor

@kylebebak kylebebak commented Apr 26, 2019

Closes #6930.

Allows users to run and properly build TypeScript projects even if there are TypeScript type check errors.

Motivation and discussion in this issue.

I know it works because I took the react-scripts and react-dev-utils deps from the create-react-app monorepo, edited them with the same changes that are in this PR, and put them in my own repos.

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.

{
  "name": "cra-typescript",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@types/jest": "24.0.11",
    "@types/node": "11.13.7",
    "@types/react": "16.8.14",
    "@types/react-dom": "16.8.4",
    "react": "^16.8.6",
    "react-dom": "^16.8.6",
    "react-scripts": "fortana-co/react-scripts#master",
    "typescript": "3.4.5"
  },
  "scripts": {
    "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",
    "test": "react-scripts test --env=jsdom --modulePaths=src",
    "eject": "react-scripts eject"
  },
  "eslintConfig": {
    "extends": "react-app"
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  }
}

After this you'll be be able to run npm start and npm run build successfully, even if there are type errors. These type errors are printed to the terminal and the browser console.

…build TypeScript projects even if there are TypeScript type check errors
@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@stale
Copy link

stale bot commented May 26, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label May 26, 2019
@kylebebak
Copy link
Contributor Author

@iansu @Timer @amyrlam @ianschmitz

Hey all! The bot marked this as stale... Any chance I could get some feedback? I think the way TS errors are currently handled is a pain point that's worth fixing, and this does it in a way I think is sensible.

@stale stale bot removed the stale label May 27, 2019
@ignacioureta
Copy link

Please consider this ticket. we have a large TS codebase and without this we can't migrate without a huge burdain

@bugzpodder bugzpodder added this to the 3.0.2 milestone Jun 19, 2019
@bugzpodder bugzpodder modified the milestones: 3.0.2, 3.x, 3.1 Jun 19, 2019
@Yankovsky
Copy link

The issue is a huge pain for us as well. Currently we are forced to add custom css that hides error overlay.

@mrmckeb mrmckeb self-assigned this Jul 15, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Jul 15, 2019

Hi all, I have to say that I'm personally not 100% in favour of this - as I feel it could be abused. However, I also understand the concerns around migration.

As this PR solves this and still warns, I think it's OK.

I'll need a second approval though - once the issues are fixed ;)

process.exit(1);
const tscCompileOnError = process.env.TSC_COMPILE_ON_ERROR === 'true';
if (tscCompileOnError) {
console.log(chalk.yellow('Compiled with warnings.\n'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should state here that due to the flag, these warnings may be serious. Or just say that there were errors that the user has chosen to ignore with the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we could make it more obvious that there were type errors on compilation. How about this:

console.log(chalk.red(
  'Compiled with the following type errors (you may want to check these before deploying your app):\n'
));

packages/react-dev-utils/README.md Show resolved Hide resolved
…nsole when an app is built in presence of type errors
@kylebebak
Copy link
Contributor Author

@mrmckeb

Thanks for reviewing this... I think it's ready to go now!

@kylebebak
Copy link
Contributor Author

@mrmckeb @ianschmitz

TSC_COMPILE_ON_ERROR is now documented in advanced-configuration. Is there anything else to do, or can we get this merged?

Thanks!

@artalar
Copy link

artalar commented Aug 24, 2019

@ianschmitz @mrmckeb please, finish this PR, many people really need this feature.

@kylebebak
Copy link
Contributor Author

@rovansteen @ianschmitz @mrmckeb

Hey guys, just bumping again. As far as I can tell this is ready to merge, and I just wanted to confirm that that's the case. If it's not, please let me know what else it needs. Thank you!

@kylebebak
Copy link
Contributor Author

kylebebak commented Sep 23, 2019

@rovansteen @ianschmitz @mrmckeb

Hey guys, bumping again... This PR is ready to go, and lots of people would like to see it merged.

If there's something that's preventing it from being merged it would be nice to hear what that is. If there's not, please merge it or let us know when that might happen!

@RDIL
Copy link
Contributor

RDIL commented Sep 23, 2019

+1, please get this in asap

@RDIL
Copy link
Contributor

RDIL commented Sep 26, 2019

@mrmckeb can we get this merged now?

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, I just have some small comments on the documentation.

@@ -29,7 +29,9 @@ yarn add typescript @types/node @types/react @types/react-dom @types/jest

Next, rename any file to be a TypeScript file (e.g. `src/index.js` to `src/index.tsx`) and **restart your development server**!

Type errors will show up in the same console as the build one.
Type errors will show up in the same console as the build one. By default, Create React App prevents you from running the dev server if your code has type errors. If you introduce type errors to your project, you have to fix or ignore them before you continue development.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather we omit this for now, as we don't want users to do this unless they absolutely must. Perhaps just as small snippet like "for advanced configuration, see (advanced config)" - as a terrible example.

That way, people can find it if they need, but they won't assume it's a normal practice to use this.

Ideally, this should never be used unless you're migrating a lot of legacy code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mrmckeb

I've changed the above to the following:

Type errors will show up in the same console as the build one. You'll have to fix these type errors before you continue development or build your project. For advanced configuration, see here.

Now it doesn't explicitly or even implicitly encourage anyone to do this =)

Thanks for your feedback!

Type errors will show up in the same console as the build one.
Type errors will show up in the same console as the build one. By default, Create React App prevents you from running the dev server if your code has type errors. If you introduce type errors to your project, you have to fix or ignore them before you continue development.

You can remove this restriction by running your app with the `TSC_COMPILE_ON_ERROR` environment variable set to `true`, for example, by adding `TSC_COMPILE_ON_ERROR=true` as documented in our [advanced configuration](advanced-configuration.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to say that we don't recommend this. @ianschmitz, what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I agree this is likely an advanced scenario for those porting legacy code. New code bases really shouldn't be using this option.

@ianschmitz ianschmitz requested review from ianschmitz and removed request for bugzpodder September 28, 2019 22:37
@mrmckeb
Copy link
Contributor

mrmckeb commented Sep 30, 2019

@RDIL If you can make these last changes, I can merge this in. Thanks!

@RDIL
Copy link
Contributor

RDIL commented Sep 30, 2019

I would love to, but this is @kylebebak 's PR, not mine!

@kylebebak
Copy link
Contributor Author

@ianschmitz @mrmckeb @RDIL

Hey all, I made the suggested changes.

Thanks for reviewing... Looks like this is ready to go!

@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 1, 2019

Sorry @RDIL, I got confused between this and another PR. Multitasking fail.

Thanks @kylebebak!

@mrmckeb mrmckeb closed this Oct 1, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 1, 2019

Trying to fix the CI.

@mrmckeb mrmckeb reopened this Oct 1, 2019
@mrmckeb mrmckeb merged commit 71946b1 into facebook:master Oct 1, 2019
@ianschmitz ianschmitz modified the milestones: 3.2, 3.1.3 Oct 1, 2019
@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 this pull request may close these issues.

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