GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Navto covers all projects by sandersn · Pull Request #38027 · microsoft/TypeScript

Conversation

@sandersn

@sandersn Nathan Shively-Sanders (sandersn) commented Apr 17, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Nathan Shively-Sanders (sandersn) added 4 commits April 16, 2020 11:01
@sandersn
Remove needless structure/destructuring
b48a724 
Just pass multiple arguments! Sheesh!
@sandersn
Basic working prototype
40e80ca
@sandersn
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.
@sandersn
remove temp notes
fc8bc8b
@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

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 Sheetal Nandi (@sheetalkamat), Andrew Casey (@amcasey), Matt Bierner (@mjbvz), Mine Starks (@minestarks) for you. Feel free to loop in other consumers/maintainers if necessary
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot TypeScript Bot (typescript-bot) assigned Nathan Shively-Sanders (sandersn) Apr 17, 2020
@typescript-bot TypeScript Bot (typescript-bot) added the Author: Team label Apr 17, 2020
@sandersn
Nathan Shively-Sanders (sandersn) requested review from Andrew Casey (amcasey) and Sheetal Nandi (sheetalkamat) April 17, 2020 17:05
sandersn
Nathan Shively-Sanders (sandersn) commented Apr 17, 2020
View reviewed changes
Comment thread src/server/session.ts Outdated

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

@sandersn Nathan Shively-Sanders (sandersn) Apr 17, 2020

Copy link
Copy Markdown
Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mjbvz
Matt Bierner (mjbvz) approved these changes Apr 17, 2020
View reviewed changes

@mjbvz Matt Bierner (mjbvz) left a comment

Copy link
Copy Markdown
Contributor

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.

Server API change look good to me

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@minestarks

Mine Starks (minestarks) commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

Ben Lichtman (@uniqueiniquity) to review protocol. I think VS passes a project so removing that may be problematic
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
uniqueiniquity
Ben Lichtman (uniqueiniquity) reviewed Apr 17, 2020
View reviewed changes

@uniqueiniquity Ben Lichtman (uniqueiniquity) left a comment

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/server/session.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
@amcasey

Andrew Casey (amcasey) commented Apr 17, 2020

Copy link
Copy Markdown
Member 
    projectFileName?: string;

Did this used to be redundant?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
amcasey
Andrew Casey (amcasey) reviewed Apr 18, 2020
View reviewed changes
Comment thread src/server/session.ts Outdated

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

@amcasey Andrew Casey (amcasey) Apr 18, 2020

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 20, 2020

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Apr 21, 2020

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Apr 21, 2020

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
amcasey
Andrew Casey (amcasey) approved these changes Apr 18, 2020
View reviewed changes

@amcasey Andrew Casey (amcasey) left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
uniqueiniquity
Ben Lichtman (uniqueiniquity) approved these changes Apr 18, 2020
View reviewed changes
sheetalkamat
Sheetal Nandi (sheetalkamat) requested changes Apr 20, 2020
View reviewed changes
Comment thread src/server/session.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/server/session.ts Outdated

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

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 20, 2020

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/testRunner/unittests/tsserver/declarationFileMaps.ts Outdated
verifyATsConfigOriginalProject(session);
});

it("navigateToAll", () => {

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 20, 2020

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sandersn
Single-project hits if projectFileName is provided
600eb8a 
and file is not
sheetalkamat
Sheetal Nandi (sheetalkamat) reviewed Apr 21, 2020
View reviewed changes
Comment thread src/server/session.ts Outdated
if (!args.file) {
if (args.projectFileName) {
const project = this.projectService.findProject(args.projectFileName);
if (project) {

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 21, 2020

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sandersn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
sheetalkamat
Sheetal Nandi (sheetalkamat) reviewed Apr 21, 2020
View reviewed changes
Comment thread 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);

@sheetalkamat Sheetal Nandi (sheetalkamat) Apr 21, 2020

Copy link
Copy Markdown
Member

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.

Why not navigateToItemIsEqualTo

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sandersn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@sandersn
use original code as fallback
9b218d6
sheetalkamat
Sheetal Nandi (sheetalkamat) approved these changes Apr 21, 2020
View reviewed changes
@sandersn
Nathan Shively-Sanders (sandersn) merged commit d571a09 into master Apr 21, 2020
@sandersn
Nathan Shively-Sanders (sandersn) deleted the navto-covers-all-projects branch April 21, 2020 22:20
@amcasey Andrew Casey (amcasey) mentioned this pull request May 12, 2020
NavTo seems slower #38491
Closed
@sheetalkamat Sheetal Nandi (sheetalkamat) mentioned this pull request Jul 23, 2020
Fixes confusing logs and unnecessary configFile checks from navto by keeping project with result. #39721
Merged
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sheetalkamat Sheetal Nandi (sheetalkamat) sheetalkamat approved these changes
+3 more reviewers
@uniqueiniquity Ben Lichtman (uniqueiniquity) uniqueiniquity approved these changes
@amcasey Andrew Casey (amcasey) amcasey approved these changes
@mjbvz Matt Bierner (mjbvz) mjbvz approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@sandersn Nathan Shively-Sanders (sandersn)

Labels

Author: Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TSServer: navto command should be supported without a file

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@sandersn @typescript-bot @minestarks @amcasey @sheetalkamat @uniqueiniquity @mjbvz
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.