Stop looking for the default configured project at node_modules#35011
Stop looking for the default configured project at node_modules#35011amcasey merged 3 commits intomicrosoft:masterfrom
Conversation
|
The tests pass - it's just a lint error. |
src/server/editorServices.ts
Outdated
| findDefaultConfiguredProject(info: ScriptInfo) { | ||
| if (!info.isScriptOpen()) return undefined; | ||
| const configFileName = this.getConfigFileNameForFile(info); | ||
| const configFileName = this.getConfigFileNameForFile(info, /*stopAtNodeModules*/ false); |
There was a problem hiding this comment.
@sheetalkamat Would it be better to just always stop at node_modules? I couldn't figure out whether introducing this inconsistency to maintain back compat would cause problems.
There was a problem hiding this comment.
I think instead of changing how we look for default config file name, we should change assignProjectToOpenedScriptInfo to not create configured project if default configured project is not loaded and script info has /node_modules/ in its path and its not orphan
There was a problem hiding this comment.
I passed false here to retain the old behavior. assignProjectToOpenedScriptInfo passes true, as does reloadConfiguredProjectForFiles.
I'm not sure I understand the orphan check in general or how it applies to this case. I think, roughly, you mean that we might want to do it if the node_modules file isn't in some other project, but I'm not sure I agree - it seems like we never want to load a configured project for a node_modules file.
There was a problem hiding this comment.
Well if that's the case why look for default configured project for such script infos in first place That is if block completely at https://github.com/microsoft/TypeScript/blob/master/src/server/editorServices.ts#L2670 completely?
There was a problem hiding this comment.
The reason I made the change at a lower level than assignProjectToOpenedScriptInfo was because I thought other consumers of getConfigFileNameForFile might need to be updated as well. For example, it seems like reloadConfiguredProjectForFiles should also not consider the default configured project.
There was a problem hiding this comment.
@sheetalkamat I don't feel strongly about how this should be implemented. If you have a different approach in mind, please let me know and I'll do my best to adopt it.
amcasey
force-pushed
the
NodeModulesProjectRoot
branch
from
8d35b16 to
6515d37
Compare
|
Just hit the same issue in material-ui, so rebasing and promoting from draft. |
|
FYI @ahejlsberg, this was the pause we saw when we invoked go-to-def on |
amcasey
changed the title
…fault configured projects
| result = action(jsconfigFileName, combinePaths(canonicalSearchPath, "jsconfig.json")); | ||
| if (result) return jsconfigFileName; | ||
|
|
||
| // If we started within node_modules, don't look outside node_modules. |
There was a problem hiding this comment.
Can you please add tests so we have coverage for this.
There was a problem hiding this comment.
Sorry, this was originally a draft PR to get feedback and I forgot to add them when I promoted it. Will do.
When searching for a default configured project, stop at `node_modules`.
3 participants
Should help with #34843.
Basically, if go-to-defintion causes a node_modules file to be opened, it should probably not trigger loading of a new project (typically, the one at the project root, since that's where node_modules is).