Navto covers all projects #38027

sandersn · 2020-04-17T17:05:03Z

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

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.

typescript-bot · 2020-04-17T17:05:05Z

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

sandersn · 2020-04-17T17:06:13Z

src/server/session.ts

@@ -1895,14 +1900,27 @@ namespace ts.server {

        private getFullNavigateToItems(args: protocol.NavtoRequestArgs): readonly NavigateToItem[] {
            const { currentFileOnly, searchValue, maxResultCount } = args;
+            if (!args.file) {


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

Server API change look good to me

minestarks · 2020-04-17T20:06:16Z

@uniqueiniquity to review protocol. I think VS passes a project so removing that may be problematic

uniqueiniquity

In general, this change is desired by VS, so definitely on board with the new functionality 👍

src/server/session.ts

amcasey · 2020-04-17T23:58:38Z

    projectFileName?: string;

Did this used to be redundant?

Refers to: src/server/protocol.ts:2807 in fc8bc8b. [](commit_id = fc8bc8b, deletion_comment = False)

amcasey · 2020-04-18T00:23:38Z

src/server/session.ts

@@ -1895,14 +1900,27 @@ namespace ts.server {

        private getFullNavigateToItems(args: protocol.NavtoRequestArgs): readonly NavigateToItem[] {
            const { currentFileOnly, searchValue, maxResultCount } = args;
+            if (!args.file) {


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.

Agree, With no file and just project, the result should be only from that project.

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.)

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

LGTM, unless there's some project loading subtlety I don't understand.

src/server/session.ts

sheetalkamat · 2020-04-20T18:37:27Z

src/server/session.ts

@@ -1895,14 +1900,27 @@ namespace ts.server {

        private getFullNavigateToItems(args: protocol.NavtoRequestArgs): readonly NavigateToItem[] {
            const { currentFileOnly, searchValue, maxResultCount } = args;
+            if (!args.file) {


Agree, With no file and just project, the result should be only from that project.

sheetalkamat · 2020-04-20T18:39:42Z

src/testRunner/unittests/tsserver/declarationFileMaps.ts

@@ -322,6 +324,46 @@ namespace ts.projectSystem {
            verifyATsConfigOriginalProject(session);
        });

+        it("navigateToAll", () => {


Need test when file is not specified and project is...

and file is not

sheetalkamat · 2020-04-21T18:47:18Z

src/server/session.ts

+            if (!args.file) {
+                if (args.projectFileName) {
+                    const project = this.projectService.findProject(args.projectFileName);
+                    if (project) {


Throw No Project here as it should be error to send project that does not exist

sheetalkamat · 2020-04-21T18:49:08Z

src/server/session.ts

+                return combineProjectOutputFromEveryProject(
+                    this.projectService,
+                    project => project.getLanguageService().getNavigateToItems(searchValue, maxResultCount, /*filename*/ undefined, /*excludeDts*/ project.isNonTsProject()),
+                    (a, b) => a.fileName === b.fileName);


Why not navigateToItemIsEqualTo

sandersn added 4 commits April 16, 2020 11:01

Remove needless structure/destructuring

b48a724

Just pass multiple arguments! Sheesh!

Basic working prototype

40e80ca

Cleaned up version

e212f1e

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.

remove temp notes

fc8bc8b

typescript-bot assigned sandersn Apr 17, 2020

typescript-bot added the Author: Team label Apr 17, 2020

sandersn requested review from sheetalkamat and amcasey April 17, 2020 17:05

sandersn commented Apr 17, 2020

View reviewed changes

mjbvz approved these changes Apr 17, 2020

View reviewed changes

uniqueiniquity reviewed Apr 17, 2020

View reviewed changes

src/server/session.ts Outdated Show resolved Hide resolved

amcasey reviewed Apr 18, 2020

View reviewed changes

amcasey approved these changes Apr 18, 2020

View reviewed changes

uniqueiniquity approved these changes Apr 18, 2020

View reviewed changes

sheetalkamat requested changes Apr 20, 2020

View reviewed changes

Single-project hits if projectFileName is provided

600eb8a

and file is not

sheetalkamat reviewed Apr 21, 2020

View reviewed changes

use original code as fallback

9b218d6

sheetalkamat approved these changes Apr 21, 2020

View reviewed changes

sandersn merged commit d571a09 into master Apr 21, 2020

sandersn deleted the navto-covers-all-projects branch April 21, 2020 22:20

amcasey mentioned this pull request May 12, 2020

NavTo seems slower #38491

Closed

sheetalkamat mentioned this pull request Jul 23, 2020

Fixes confusing logs and unnecessary configFile checks from navto by keeping project with result. #39721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navto covers all projects #38027

Navto covers all projects #38027

sandersn commented Apr 17, 2020

typescript-bot commented Apr 17, 2020

sandersn Apr 17, 2020

mjbvz left a comment

minestarks commented Apr 17, 2020

uniqueiniquity left a comment

amcasey commented Apr 17, 2020

amcasey Apr 18, 2020

sheetalkamat Apr 20, 2020

sandersn Apr 21, 2020

sandersn Apr 21, 2020

amcasey left a comment

sheetalkamat Apr 20, 2020

sheetalkamat Apr 20, 2020

sheetalkamat Apr 21, 2020

sheetalkamat Apr 21, 2020

Navto covers all projects #38027

Navto covers all projects #38027

Conversation

sandersn commented Apr 17, 2020

typescript-bot commented Apr 17, 2020

Choose a reason for hiding this comment

mjbvz left a comment

Choose a reason for hiding this comment

minestarks commented Apr 17, 2020

uniqueiniquity left a comment

Choose a reason for hiding this comment

amcasey commented Apr 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment