-
Notifications
You must be signed in to change notification settings - Fork 29.3k
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 tsconfig resolution for navigation #192851
Conversation
@mjbvz can you take a look and approve these changes if they look ok? |
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.
Works as expected!
@@ -154,7 +154,7 @@ async function getTsconfigPath(baseDirUri: vscode.Uri, pathValue: string): Promi | |||
return absolutePath; | |||
} | |||
return absolutePath.with({ | |||
path: `${absolutePath.path}.json` | |||
path: `${absolutePath.path}/tsconfig.json` |
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.
From https://github.com/microsoft/TypeScript/blob/febfd442cdba343771f478cf433b0892f213ad2f/src/compiler/commandLineParser.ts#L3005 I think we need to check both PATH.json
and PATH/tsconfig.json
, at least when resolving the path in extends
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.
Updated. From my own testing and looking at typescript documentation, extends and references are different in resolution. I have added a linkType
parameter to the navigation command to resolve this, though we can also create a separate command if necessary.
Thanks @ronakj! This will be in the next VS code insiders build |
* Fix tsconfig resolution * Resolve based on link type
Fixes #192772
Tested following scenarios -
node_modules
Tested on Linux & Windows