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

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

Merged
merged 5 commits into from
Jan 3, 2024

Conversation

dominikg
Copy link
Owner

@dominikg dominikg commented Jan 1, 2024

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
Copy link
Owner Author

dominikg commented Jan 1, 2024

this makes tsconfck a bit slower but more correct.

@dominikg
Copy link
Owner Author

dominikg commented Jan 2, 2024

@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
Copy link
Sponsor Contributor

bluwy commented Jan 3, 2024

@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
Copy link
Owner Author

dominikg commented Jan 3, 2024

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.

@dominikg dominikg merged commit 5139758 into main Jan 3, 2024
5 checks passed
@dominikg dominikg deleted the fix/extends2 branch January 3, 2024 08:55
@github-actions github-actions bot mentioned this pull request Jan 3, 2024
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.

parse function resolves "extends": ".." incorrectly
2 participants