Skip to content

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

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

Merged
merged 1 commit into from
Jul 24, 2020

Conversation

sheetalkamat
Copy link
Member

This fixes multiple issues:

  1. When combining result, we now keep project it got the result from so that can be used to map the result correctly if some kind of manipulation is needed.
  2. Info now logs correct information about 'loading' and not 'reloading' when solution project is loaded (because it was just created and not loaded when file was opened to delay that as long as it is needed)
  3. default config file name is cached for the file instead of having to recalculate it everytime someone asks for default project for open file.
    Fixes NavTo seems slower #38491

Also cache the result of config file name for open files
Fixes #38491
@sheetalkamat sheetalkamat requested review from amcasey and sandersn July 23, 2020 23:24
@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Jul 23, 2020
@sheetalkamat
Copy link
Member Author

sheetalkamat commented Jul 23, 2020

@amcasey @mjbvz @sandersn its not clear if we need to change behaviour to only currently open projects instead of looking in all projects like findAllRefs is suppose to do for navTo, @sandersn added this as part of #38027 and i am not sure if this need to change and if yes how

Btw to me #38027 seems the right behaviour. People want to search in their workspace as opposed to open projects i think!

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

This definitely seems like it would improve performance (did we measure?) but I agree that further discussion is needed around what we actually want FAR to do in these scenarios. I agree that the behavior in #38027 matches my expectations for how the feature should work, but I believe it was originally motivated by a request from @mjbvz and I had the impression that it might not have matched his expectations.

@sheetalkamat sheetalkamat changed the title Fixes slow nav to issue by keeping project with result. Fixes confusing logs and unnecessary configFile checks from navto by keeping project with result. Jul 24, 2020
@sheetalkamat
Copy link
Member Author

@amcasey I think the title was log. I only picked up title from the issue but as issue explains the slowness (if any is expected and controlled by the vscode setting with scope for navTo) but the logs were confusing and could add little bit delay to get config file name for each nav to result's fileName.
I havent measured perf but it wont be severe, but this just helps overall since all semantic languages need to look for default config file => default configured project for a info.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

I think the change is good, but I'm not convinced we should close the linked bug.

@sheetalkamat
Copy link
Member Author

Will open new one to discuss the behaviour about opening the issue.

@sheetalkamat sheetalkamat merged commit 823b69a into master Jul 24, 2020
@sheetalkamat sheetalkamat deleted the slowNavTo branch July 24, 2020 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NavTo seems slower
3 participants