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 tsconfig resolution for navigation #192851

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

ronakj
Copy link
Contributor

@ronakj ronakj commented Sep 12, 2023

Fixes #192772

Tested following scenarios -

  1. Relative path without json at the end
  2. Relative path with json at the end
  3. tsconfig which resolves to a module in node_modules

Tested on Linux & Windows

@ronakj ronakj changed the title Fix tsconfig resolution Fix tsconfig resolution for navigation Sep 12, 2023
@ronakj
Copy link
Contributor Author

ronakj commented Sep 18, 2023

@mjbvz can you take a look and approve these changes if they look ok?

Copy link

@ezhil56x ezhil56x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected!

@mjbvz mjbvz added this to the October 2023 milestone Oct 5, 2023
@@ -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`
Copy link
Collaborator

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

Copy link
Contributor Author

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.

@mjbvz mjbvz modified the milestones: October 2023, November 2023 Oct 24, 2023
@mjbvz mjbvz merged commit d72005b into microsoft:main Nov 6, 2023
6 checks passed
@mjbvz
Copy link
Collaborator

mjbvz commented Nov 6, 2023

Thanks @ronakj! This will be in the next VS code insiders build

amunger pushed a commit that referenced this pull request Nov 9, 2023
* Fix tsconfig resolution

* Resolve based on link type
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tsconfig.json references navigation doesn't understand folder references
4 participants