fix(host): invalidate cache less to speed up compilation for large projects

[std4453]

Summary

Fixes a few problems related to host.ts and speed up compilation for large projects.

Details

The current version of rollup-plugin-typescript2 causes the TypeScript cache to get constantly invalidated, for each entry file during a single rollup build, causing build time to increase non-linearly. For for fairly complicated projects, this easily bloat into minutes (about 400s for our project with ~50 files and ~15000 locs).

I've identified and fixed these problems that caused this:

• In setSnapshot, file version is increased unnecessarily when source hasn't changed.
• Files in node_modules are added incorrectly into LanguageServiceHost.fileNames by setSnapshot, since the function is called indirectly for all dependency files discovered while type checking. fileNames should be the list of entry files for this build, there's no point updating it during the build process.
• In getScriptSnapshot, also create snapshot when file is empty, otherwise it would be marked as 'missing' and invalidate the cache.

I've verified that the updated code can compile itself and our company project, making absolutely no changes to the output. The new compile time is 14s compared to 400s.

You can override rollup-plugin-typescript2 with @std4453/rollup-plugin-typescript2 to try it out, which is forked from version 0.36.1.

[std4453]

@agilgur5 I think that CI is failing because the latest version of actions/setup-node is now v4.

[ezolenko]

@std4453 yeah, I fixed it in master yesterday, if you merge CI should work

[agilgur5]

Yes you should be able to rebase on top of #477 to get CI to run properly.

I haven't fully reviewed this, but it might be good to add regression tests for these changes. Also some tests may need updating to account for these changes

[std4453]

@ezolenko I've rebased my branch to include the CI fix, look like it needs approval from maintainers to run.

[agilgur5]

I approved it before (when it failed due to old actions/setup-node@v2), but apparently we need to do so again after the rebase. It has ran now and errors out as expected on the host tests, which would need adjusting.

[std4453]

OK I think I've resolved all the code comments, however I couldn't get the watch CI spec working, even locally. Weirdly though, even if I revert the code I've edited, the test would still fail. Any idea on why that might happen? @agilgur5

[std4453]

I've done more digging and here's what I found:

The remaining test doesn't work because TS does not view the file as changed, therefore it won't recompile the file. This is because servicesHost is not preserved across builds under watch mode, so versions always starts with undefined.

Under watch mode when a file was updated, rollup first calls buildStart and then transform. In buildStart, rpt2 calls service.getCompilerOptionsDiagnostics, which would tell TS to sync host versions, and it invokes getScriptSnapshot for all related files, effectively setting all their versions to 1. Then, in transform, rpt2 calls setSnapshot, which originally bumps version of the updated file to 2.

However, since we now check whether source content has changed in setSnapshot, the invocation in transform simply does nothing, leaving the version of the file with 1. This means that in every build under watch mode, the version of every file stays at 1, so TypeScript never sees the need to fetch new file snapshots from host, and recompiles no files at all.

This still wouldn't be a problem if TypeScript always create its initial snapshots with latest sources, however it actually read from documentRegistry if file version is not changed, and #388 makes documentRegistry get preserved across builds, and it never get updated if getScriptVersion stays at 1.

(p.s., if the updated file get updated again, it looks like the script versions would still be 1 for all other files and 2 for the updated file. This actually isn't a problem because service.getCompilerOptionsDiagnostics was called before setSnapshot, so the updated file would still have version 1, and TypeScript think that its version has changed and reloads the file. Eventually it gets reloaded again in getEmitOutput, which can be optimized but maybe next time.)

To avoid further tampering with existent logic and breaking things unexpected, I added a third argument forceUpdate to setSnapshot, which forces the bumping of version even when source is not changed. It is only passed true in transform under watch mode, so that wouldn't affect any other branch.

Under watch mode, this effectively restores the original behavior of always incrementing version in setSnapshot, which can be verified since there's only two places calling setSnapshot, and getScriptSnapshot wouldn't even call setSnapshot if there already is a snapshot for the file requested. Under build mode, source file would not change during the build process, so there's no need to update version whatsoever.

After this, all tests should be passing now.

[agilgur5]

Hey @std4453, thanks for the iterations and comments!

Just wanted to leave a heads-up that my family and I have COVID right now (again), so it's gonna be a bit before I can review this in-depth as my brain is not running too well right now 🤕😷

[std4453]

@agilgur5 Hello, is there any plan on merging this PR? If there's any concerns on its compatibility with current use-cases, I can elaborate on this.

[matthew-gladman-oua]

Can confirm that this fixed a similar issue for us; about 4 - 5 min compile time dropped to 10 seconds after this change 👍