Type checking takes too long to be a precommit hook

[samreid]

@jessegreenberg said:

Recently commits have started to take 30-40 seconds to process. I usually commit from the command line. Do our commit hooks recompile everything? If so it will probably improve with #1156

I replied:

Yes, the precommit hook runs tsc --build (used to incorrectly be just tsc). We will still want to run the type checker before committing, so I’m not sure how to speed that up. #1156 is just about speeding up transpilation, not about speeding up type checking.

@jonathanolson said:

The transputer (sic) means that it now would include a full build for tsc no?

I replied:

I didn’t follow your question @jonathanolson. But IMO running type check with --build may just be too time consuming and maybe we should shut off that precommit hook. Just test it on CT and rely on devs for testing it. OK if I turn off the type check precommit hook for now?

[samreid]

Or maybe there is a subset of repos where tsc -b runs fast enough to leave this on as a precommit hook? But part of the problem is that incremental builds take different amounts of time depending on what files changes.

[zepumph]

Perhaps there is an adhoc solution that would help eliminate the nlogn time complexity, since for multiple repos, they are all running tsc -b. Could we create a function where given a repo list L, we determine the correct dependency ordering, order the list into L', and then run tsc on each repo individually in that order to make sure each repo is only compiled a single time, and in the right order.

Is it possible that there is a library out on the internet that has done something like this before? I can't imagine we are the only typescript usage with so many endpoints for compiling.

[jonathanolson]

My impression was that if I was using the transpile (with watch), that tsc -b would regenerate chipper/dist files (unless we switched fully to transpilation-only for the files). Thus the precommit hook would need to redo all files, and it wouldn't be incremental?

[samreid]

Perhaps there is an adhoc solution that would help eliminate the nlogn time complexity, since for multiple repos, they are all running tsc -b. Could we create a function where given a repo list L, we determine the correct dependency ordering, order the list into L', and then run tsc on each repo individually in that order to make sure each repo is only compiled a single time, and in the right order.

This is precisely what --build does. It's just that when certain files are changed, there are a lot of things to type check. But I still would like to run some timing and performance tests to quantify this.

We will shut off JS file output for tsc -b and only have it output declaration files. But AFAIK the “type checking without emitting JS” can still be incremental.

[samreid]

Tagging for dev meeting to ask if it's OK if to disable type checking in precommit hooks.

[zepumph]

This is precisely what --build does. It's just that when certain files are changed, there are a lot of things to type check. But I still would like to run some timing and performance tests to quantify this.

I'm not sure I understand. If you commit to 4 repos, and they all depend on joist, all 4 of the pre-commit processes are still going to look at joist.

Tagging for dev meeting to ask if it's OK if to disable type checking in precommit hooks.

Do we feel like we are at the fastest that we can be? If you feel confident that we can't speed things up any more, then I'm fine removing it and utilizing CT, but it seems less than ideal.

[samreid]

I'm not sure I understand. If you commit to 4 repos, and they all depend on joist, all 4 of the pre-commit processes are still going to look at joist.

Yes, but our project references are set up to be incremental, so declarations for joist are only built/updated once.

Do we feel like we are at the fastest that we can be? If you feel confident that we can't speed things up any more, then I'm fine removing it and utilizing CT, but it seems less than ideal.

I haven't investigated any of the ideas in https://github.com/microsoft/TypeScript/wiki/Performance. But I think some type checks are fast and some are slow, and I don't know how to know which will be fast or slow, or why the slow ones are slow.

[pixelzoom]

Tagging for dev meeting to ask if it's OK if to disable type checking in precommit hooks.

An emphatic "no" here. Type-checking is not done by transpile.js. If it's not done in a precommit hook, then we're relying on the developer to run tsc before committing (or at least before pushing). When (not if) that doesn't happen, it will be noted by CT, but the developer needs to first notice it in CT, then fix it. But by then, it may be impacting the work of multiple developers.

[samreid]

If it's not done in a precommit hook, then we're relying on the developer to run tsc before committing (or at least before pushing)

Keeping the type check sounds reasonable to me. And if the developer has already run a type check, that will be cached (since we use incremental tsc) and the type check during the precommit hook will be immediate.

Recently commits have started to take 30-40 seconds to process.

I have not had this experience at all, my commits are running quickly for my current work. Maybe we can start taking notes on "which repos" and "which kind of changes" trigger that long type check?

[samreid]

We may optimize this by implementing #1174 -- having one watch process, and the precommit hook just "checks in" with the watch process to see (a) is it running and (b) is it up-to-date with no errors? I also realized:

I also realized that if you change code in e.g., sun you need to run the type checker not just on sun and its dependencies, but on things that use sun (things that have sun as a dependency) so you can make sure they didn’t break.

[samreid]

We discussed it today, we agreed it is OK to run the full type check as a precommit hook. We like the fast transpilation, and this seems like a reasonable compromise.