-
Notifications
You must be signed in to change notification settings - Fork 207
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
fix: Only build typescript files in pre-commit for typescript projects #142
fix: Only build typescript files in pre-commit for typescript projects #142
Conversation
Codecov Report
@@ Coverage Diff @@
## master #142 +/- ##
==========================================
- Coverage 87.65% 86.90% -0.75%
==========================================
Files 18 18
Lines 332 336 +4
Branches 78 80 +2
==========================================
+ Hits 291 292 +1
- Misses 34 36 +2
- Partials 7 8 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! Thank you for this fix! One question.
@@ -134,7 +135,8 @@ If you customised your `.babelrc`-file you might need to manually add | |||
`kcd-scripts` will automatically load any `.ts` and `.tsx` files, including the | |||
default entry point, so you don't have to worry about any rollup configuration. | |||
|
|||
`tsc --noemit` will run during lint-staged to verify that files will compile. | |||
`tsc --build tsconfig.json` will run during before committing to verify that files will compile. | |||
So make sure to add the `noEmit` flag to the `tsconfig.json`'s `compilerOptions`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for this requirement to add noEmit to the config? Could we not use the flag in the command instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, no.
When you build a typescript project, you have two options:
- Use a tsconfig.json file (that's what we do here)
- Provide all compiler options as command line options
And you can't mix those two.
So running something like tsc --noEmit --build tsconfig.json
will not work (error TS6369: Option '--build' must be the first command line argument.
) and switching noEmit and build will also not work (error TS5072: Unknown build option '--noEmit'.
).
I thought about creating a custom tsconfig file just before building the ts project. It could use the extends
field to include all the configuration of the project's tsconfig. But I'm not sure if this is that reliable. For example it needs to be removed again after building the project. But what happens if the user interrupts the script during the ts build? These cases would need to be handled. Handling that would require quite some code. And I'm not sure if adding this code is really worth it. Maybe having this hint (i.e. "please add --noEmit to your tsconfig file") is enough. After all, you will want to have this option set anyways if you use babel as a typescript compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think eventually we'll have a tsconfig in kcd-scripts that my projects will extend. But for now this is acceptable. Thanks!
Wouldn't this be fixed by just calling the |
actually, I think it's better to run the typescript compiler across the entire code base not just the ones that changed. So I think this is a good change. |
Hey Kent, I just wanted to ping you that I answered to your review comment. I don't know if GitHub sends you an email for that. @MichaelDeBoey Yes, it would be fixed by calling the |
@all-contributors please add @Lukas-Kullmann for code and docs |
I've put up a pull request to add @Lukas-Kullmann! 🎉 |
🎉 This PR is included in version 6.0.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
Currently, there is a bug in the lint-staged configuration: It checks if
ifTypescript
is truthy. Since this is a function, it will be truthy for every project. I changed the pre-commit script to move typescript compilation from lint-staged to building the whole project during pre-commit.Why:
This is a follow-up on #141 and testing-library/dom-testing-library#572. Currently, it tries to build dom testing library as a typescript project even though it is not one (it doesn't even have typescript as a dependency). This is wrong in my opinion since the reason for the ts build there is that they want to check the typescript definitions. This is already done by dtslint.
How:
I removed the lint-staged command for building typescript and added a typescript build to the pre-commit script.
Checklist: