Conversation
Just pass multiple arguments! Sheesh!
1. Add test 2. Change protocol. navto-all only happens when filename is undefined or missing. 3. Change location to earlier code path. This change was largely type-guided and resulted in some duplicated code, but I think it's less fault-prone.
|
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
src/server/session.ts
Outdated
|
|
||
| private getFullNavigateToItems(args: protocol.NavtoRequestArgs): readonly NavigateToItem[] { | ||
| const { currentFileOnly, searchValue, maxResultCount } = args; | ||
| if (!args.file) { |
There was a problem hiding this comment.
this is the only substantive change. Everything else is a removal of a frankly ridiculous amount of destructuring/allocation to pass around a project+location pair.
mjbvz
left a comment
mjbvz
left a comment
There was a problem hiding this comment.
Server API change look good to me
|
@uniqueiniquity to review protocol. I think VS passes a project so removing that may be problematic |
uniqueiniquity
left a comment
uniqueiniquity
left a comment
There was a problem hiding this comment.
In general, this change is desired by VS, so definitely on board with the new functionality 👍
src/server/session.ts
Outdated
|
|
||
| private getFullNavigateToItems(args: protocol.NavtoRequestArgs): readonly NavigateToItem[] { | ||
| const { currentFileOnly, searchValue, maxResultCount } = args; | ||
| if (!args.file) { |
There was a problem hiding this comment.
file [](start = 22, length = 4)
Seems like it's now possible to specify a project and no file. If some caller were to do that, I'd expect it to get back project.getLanguageService().getNavigateToItems.
There was a problem hiding this comment.
Agree, With no file and just project, the result should be only from that project.
There was a problem hiding this comment.
The original case handles basically this case, but includes hits from all dependencies as well. Any ideas for how to reduce it to just the current project?
(It's probably OK to keep the original behaviour, since it won't at least surprise anyone.)
There was a problem hiding this comment.
For example, in the test cases, the 'user' project that depends on 'a' and 'b' projects returns [user.ts, a.ts, b.ts] for program.getSourceFiles()
amcasey
left a comment
amcasey
left a comment
There was a problem hiding this comment.
LGTM, unless there's some project loading subtlety I don't understand.
src/server/session.ts
Outdated
|
|
||
| private getFullNavigateToItems(args: protocol.NavtoRequestArgs): readonly NavigateToItem[] { | ||
| const { currentFileOnly, searchValue, maxResultCount } = args; | ||
| if (!args.file) { |
There was a problem hiding this comment.
Agree, With no file and just project, the result should be only from that project.
| verifyATsConfigOriginalProject(session); | ||
| }); | ||
|
|
||
| it("navigateToAll", () => { |
There was a problem hiding this comment.
Need test when file is not specified and project is...
and file is not
src/server/session.ts
Outdated
| if (!args.file) { | ||
| if (args.projectFileName) { | ||
| const project = this.projectService.findProject(args.projectFileName); | ||
| if (project) { |
There was a problem hiding this comment.
Throw No Project here as it should be error to send project that does not exist
src/server/session.ts
Outdated
| return combineProjectOutputFromEveryProject( | ||
| this.projectService, | ||
| project => project.getLanguageService().getNavigateToItems(searchValue, maxResultCount, /*filename*/ undefined, /*excludeDts*/ project.isNonTsProject()), | ||
| (a, b) => a.fileName === b.fileName); |
There was a problem hiding this comment.
Why not navigateToItemIsEqualTo
7 participants
With this PR, when navto is called without a file, it returns matches from all projects. Currently it only returns matches from the current project and its dependencies.
I had to change the protocol interface for this, and I'm not sure I did that in the right way. I'm also not sure what tests to write for this feature, beyond the basic one I have.
Fixes #2530