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

Add typescript compilation check and hooks update #700

Merged
merged 4 commits into from
Oct 5, 2018

Conversation

migmartri
Copy link
Contributor

@migmartri migmartri commented Oct 4, 2018

The issue with tslint is that it actually does not check that the compilation works, from their documentation

Please ensure that the TypeScript source files compile correctly before running the linter.

So I've added a compilation step during the pre-hook. Please note that it was added in the husky hooks not in the lint-staged. This is because the compilation does not depend on the changed files (but on the tsconfig.json) so it can not be used there. More info here

Closes #694
Closes #158

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM

@prydonius
Copy link
Contributor

Won't this slow down the time to commit significantly?

@andresmgot
Copy link
Contributor

andresmgot commented Oct 5, 2018

Won't this slow down the time to commit significantly?

The command takes 8s in my local env. I think the prettier check takes more or less the same.

@migmartri
Copy link
Contributor Author

5 seconds here. I think it is acceptable. If we change our mind we can always revert it.

time yarn run ts-compile-check
yarn run v1.10.1
$ tsc -p tsconfig.json --noEmit
Done in 5.50s.

@migmartri migmartri merged commit 62538b1 into vmware-tanzu:master Oct 5, 2018
@migmartri migmartri deleted the 694-tslint-issue branch October 5, 2018 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants