feat: add opt-in twoslash build cache - #118
Conversation
🦋 Changeset detectedLatest commit: a6f2e1f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
IMax153
changed the title
|
👋 Hey @Adammatthiesen! We're currently working on the next iteration of the Effect Website, and I'm trying to optimize build times as much as possible. As you know, we make heavy use of your I've been experimenting on our new site with a build-time cache for Twoslash snippets. I locally patched the For an Astro build with my cache PoC including ~40 Twoslash snippets, the cold build time was ~25s and warm build time was ~15s. Obviously this scales with more and more snippets that are cached vs. uncached, and the actual Effect website has many more snippets than this. Given the positive results, I figured I'd make a PR to Please note that the PR is fully AI generated - I'm happy to clean it up if you're interested in upstreaming it, and also happy to make whatever changes to the public API you want too! |
|
@IMax153 Hey Max! I'm back! gonna be looking into this here soon! |
Take your time! There's no rush :) |
Adammatthiesen
left a comment
Adammatthiesen
left a comment
There was a problem hiding this comment.
Looks pretty solid to me, Let me get a second pair of eyes on it but i give my approval
There was a problem hiding this comment.
oh wow this is such an amazing work @IMax153. but found 2 things i think it would be worth to address (or at least document) before this lands:
- functions and regexes are invisible to the cache key,
normalizeJsonmaps funcs tonulland regexes fall into the generic object branch with no enumerable keys, so they can become{}. verified locally that 2 keys built with differentshouldGetHoverInfocallbacks and different regexes collide. means changing a callback intwoSlashOptionssiliently serves stale cached output.fingerprintcovers it as an escape hatch but the docs should call this out explicitly, and regexes could at least normalize viaString(value) - hoisting the tsconfig parse is a behavioir change. on main,
parseSnippetTsconfigonly runs insideif (!tsLibDirectory). now it runs unconditionally and throws if there's no config atcwdso anywone passingtsLibDirectoryexplicitly without a tsconfig goes from working build to hard crash, even with the cache disabled.
| if (value === undefined || typeof value === "function" || typeof value === "symbol") { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
this is a footgun, but worth a doc note that function-valued options don't participate in invalidation and fingerprint is the workaround.
| if (typeof value === "object") { | ||
| return Object.entries(value) | ||
| .sort(([left], [right]) => left.localeCompare(right)) | ||
| .reduce<Record<string, JsonValue>>((record, [key, entryValue]) => { | ||
| record[key] = normalizeJson(entryValue); | ||
| return record; | ||
| }, {}); | ||
| } |
There was a problem hiding this comment.
regexes land here and normalize {} since they have no enum keys. an instanceofRegExp check returning String(value) before this branch would make trigger regex changes actually bust the cache.
| const resolvedTsConfigPath = resolveTsconfigPath(cwd, tsConfigPath); | ||
| const { source: tsConfigSource, options: baseCompilerOptions } = | ||
| parseSnippetTsconfig(resolvedTsConfigPath); |
There was a problem hiding this comment.
this is what i was talking about that on main this only ran when !tsLibDirectory, so this now might crash builds for anyone passing tsLibDirectory without a tsconfig on disk, cache enabled or not. we could gate the eager parse behind !tsLibDirectory || cache or make it tolerant when the lib dir is already known.
| pluginContext: { | ||
| resolvedTsConfigPath, | ||
| tsConfigSource, | ||
| }, |
There was a problem hiding this comment.
putting the absolute path in the key kills cache portability, restore the cache dir in CI at a different checkout path and everything misses. it can also be redundant, the resolved tsLibDirectory and the actual compilerOptions passed to twoslash are already in executeOptions. i'd either make it relative to cwd or drop it and keep just tsConfigSource, this is non-blocking, it could be as follow-up.
| process.once("exit", () => { | ||
| const total = stats.hits + stats.misses; | ||
| if (total === 0 || options.logLevel === "off") { | ||
| return; | ||
| } | ||
|
|
||
| const hitRate = ((stats.hits / total) * 100).toFixed(1); | ||
| console.info( | ||
| `[twoslash-cache] hits=${stats.hits} misses=${stats.misses} writes=${stats.writes} readErrors=${stats.readErrors} writeErrors=${stats.writeErrors} hitRate=${hitRate}% dir=${options.dir}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
both dedup guards here are per-instance, didRegisterSummary lives in this closure and process.once only dedupes the same listener, but every createTwoslashCacheStats() call registers a new one. so multiple plugin instances (second EC engine, dev-server reloads) = stacked exit listeners, and past 10 node prints MaxListenersExceededWarning, which looks like a leak.
a fix would be setting a module-level registry, one shared exit listener that walks all registered stats:
const registry: RegisteredStats[] = [];
let didRegisterExitListener = false;
function registerForSummary(entry: RegisteredStats) {
registry.push(entry);
if (didRegisterExitListener) return;
didRegisterExitListener = true;
process.once("exit", () => {
for (const { stats, options } of registry) {
// same summary log as now, per entry
}
});
}Important
just hoisting the boolean isn't enough. first instance's closure only sees its own stats, everyone else's counters silently vanish. the registry keeps each instance's line (with its dir=) intact. not a blocker
P.D. this isn't a blocker, but ideally if you guys don't want noise in the logs, this would do it.
3 participants
Description
pnpm changeset.Docs