dominikg / tsconfck

A utility to find and parse tsconfig files without depending on typescript
Other
290 stars 14 forks source link

fix(parse): fix edge cases for extends .. and nested extend #150

Closed dominikg closed 8 months ago

dominikg commented 8 months ago

fixes #149

Adds special case handling for '..'. The test for it revealed another bug, namely that extended tsonfigs were parsed without parsing their extends , which lead to unextended return from cache.

To avoid eager deep extend parse, it checks if extends wasn't parsed and does it on first access

dominikg commented 8 months ago

this makes tsconfck a bit slower but more correct.

dominikg commented 8 months ago

@bluwy @sapphi-red could need an eye or two here. Once released, vite needs to update its bundled tsconfck and unfortunately remove the direct cache access optimization here: https://github.com/vitejs/vite/blob/d2aa0969ee316000d3b957d7e879f001e85e369e/packages/vite/src/node/plugins/esbuild.ts#L451 so that nested extends is always resolved.

This does not affect many users today, it requires that a/b/tsconfig.json extends a/tsconfig.json extends sometsconfig and a/foo.ts being processed aftera/b/bar.ts. But it is a sneaky issue as it does not error, it just returns the "unextended" a/tsconfig.json without the values from sometsconfig.

Note unlike mentioned in #149 i did not add fs checking, opting for a simpler special case, keeping the require.resolve. In my local benchmarking this was faster and is less code.

bluwy commented 8 months ago

@bluwy @sapphi-red could need an eye or two here. Once released, vite needs to update its bundled tsconfck and unfortunately remove the direct cache access optimization here: vitejs/vite@d2aa096/packages/vite/src/node/plugins/esbuild.ts#L451 so that nested extends is always resolved.

Seems fine to me. IIUC tsconfck's parse() also checks the cache first, so the perf difference isn't big.


WRT the fix. Is the fix only supporting extends one layer deeper? Or should it work recursively instead? I'm not too familiar with the code though.

dominikg commented 8 months ago

parseExtends works deep, but is only called eager for the first config in the chain. With the new check and lazy parseExtends the return of parse always has extended set and values merged.

the new nested fixture and additional expects in the cache test validate it.