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

[WIP] TypeScript final touches #5514

Closed
wants to merge 1 commit into from

Conversation

brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Oct 21, 2018

I'll push my attempts here. Help needed. cc @Timer

@brunolemos
Copy link
Contributor Author

brunolemos commented Oct 22, 2018

#5515, #5516, #5519

@ianschmitz
Copy link
Contributor

@Timer we went through the same issues on #5509 in create-react-app-typescript.

The fix implemented in 7231285 works great when developing in the source files, however it means when working within the test files, they no longer are using the configuration within tsconfig.json.

I initially was thinking that we could potentially exclude those files via config merging when configuring ForkTsCheckerWebpackPlugin. However it seems that their config only accepts a path to the tsconfig.json.

It may be worth a little more thinking here?

@Timer
Copy link
Contributor

Timer commented Oct 22, 2018

For now, I'm OK with excluding test files. I wish it was more like awesome-typescript-loader which allows you to override configuration values on the fly. Do you have a good proposition? We can't let test files fail the build -- it should only fail tests.

@ianschmitz
Copy link
Contributor

I totally agree. I know from personal experience in create-react-app-typescript that it is super annoying to have changes to the source code start yelling at you about errors in your unit tests when running npm start. It is a continuous source of pain among our team.

I tend to agree with @frankwallis's comment over at #4837 (comment). I wonder if allowing configuration overrides when configuring the plugin would solve the issue of ignoring test build errors (configure an exclude to ignore all tests files in the tsconfig).

/cc @johnnyreilly

@brunolemos brunolemos changed the title [WIP] TypeScript finishes [WIP] TypeScript final touches Oct 22, 2018
@brunolemos brunolemos deleted the typescript branch October 22, 2018 17:59
@johnnyreilly
Copy link
Contributor

I totally agree. I know from personal experience in create-react-app-typescript that it is super annoying to have changes to the source code start yelling at you about errors in your unit tests when running npm start. It is a continuous source of pain among our team.

tend to agree with @frankwallis's comment over at #4837 (comment). I wonder if allowing configuration overrides when configuring the plugin would solve the issue of ignoring test build errors (configure an exclude to ignore all tests files in the tsconfig).

@ianschmitz - fork-ts-checker-webpack-plugin awaits your pull request 😄

Quite seriously; we're completely open to making changes that will improve the developer experience. Feel free to experiment and share.

@ianschmitz
Copy link
Contributor

ianschmitz commented Oct 22, 2018

@Timer what are your thoughts? I don't mind implementing something over at fork-ts-checker if that would help. I originally thought of the idea after having used ts-loader. They have an option that is quite handy that lets you override your configuration that you specified in tsconfig.json: https://github.com/TypeStrong/ts-loader#compileroptions-object-default.

We have used that option internally on projects, where we wanted to change the target from es5 to es2017 when running npm start so it emits a much nicer output (doesn't have all the async/await generator junk that is hard to debug).

@Timer
Copy link
Contributor

Timer commented Oct 22, 2018

Adding a compilerOptions options feature to fork-ts-checker-webpack-plugin that overrides would be great.

@ianschmitz
Copy link
Contributor

👍 I'll get on that tonight!

@ianschmitz
Copy link
Contributor

Argh I totally had a brain fart. I forgot that exclude isn't part of compilerOptions 😢. Realized it when implementing #5537.

That being said i've added the feature to fork-ts-checker over at TypeStrong/fork-ts-checker-webpack-plugin#173. May be useful to us in other ways! 😃

@lock lock bot locked and limited conversation to collaborators Jan 18, 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.

5 participants