-
Notifications
You must be signed in to change notification settings - Fork 36.8k
Add tsconfig path resolution following Node's semantics #282969
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements Node.js package.json exports field resolution for TypeScript configuration file path resolution, following the official Node.js ESM resolution algorithm. This enables VS Code to correctly resolve tsconfig.json extends paths when packages use conditional exports or subpath patterns, matching the behavior of Node.js and TypeScript.
Key Changes
- Added a complete implementation of Node.js package exports resolution algorithm with support for conditional exports, wildcard patterns, and array fallbacks
- Integrated exports resolution into the existing tsconfig path resolution flow, attempting exports-based resolution before falling back to traditional node_modules resolution
- Included comprehensive unit and integration tests to verify correctness against Node.js specification
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
extensions/typescript-language-features/src/utils/packageExports.ts |
New file implementing the Node.js package exports resolution algorithm with functions for pattern matching, conditional resolution, and validation according to Node.js spec |
extensions/typescript-language-features/src/languageFeatures/tsconfig.ts |
Adds exports-based module resolution functions (parseNodeModuleSpecifier, findNodeModulePackageRoot, tryReadPackageJson, resolveNodeModulePathUsingExports) and integrates them into getTsconfigPath |
extensions/typescript-language-features/src/test/unit/packageExports.test.ts |
New unit tests for the core exports resolution algorithm covering exact matches, wildcards, conditionals, arrays, null exclusions, and validation edge cases |
extensions/typescript-language-features/src/test/unit/tsconfigExportsResolver.test.ts |
New integration tests using the VS Code file system API to test end-to-end resolution with actual package.json files and node_modules structure |
extensions/typescript-language-features/src/utils/packageExports.ts
Dismissed
Show dismissed
Hide dismissed
mjbvz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this!
I agree it's a lot of code for a smaller bug. Let me try reaching out to TypeScript to see if they would like to take ownership of tsconfig intellisense with TS-go. That would likely provide the best user experience since it would exactly match how TS resolves these files
extensions/typescript-language-features/src/languageFeatures/tsconfig.ts
Show resolved
Hide resolved
| await vscode.workspace.fs.createDirectory(baseDir); | ||
|
|
||
| const packageRoot = vscode.Uri.joinPath(root, 'node_modules', 'pkg'); | ||
| await vscode.workspace.fs.createDirectory(vscode.Uri.joinPath(packageRoot, 'configs')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the tests need to use the file system, can we mock it out instead? I've found using the real FS is a very common source of flaky tests and slowness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mocking the FS here is quite some work since TestFS does not appear to be available in extension tests. I can do this, but would like to confirm this will be going in.
Fixes #276592