Skip to content
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

Scratch: How much can we save by not tracking or enforcing search order? #47963

Closed
wants to merge 4 commits into from

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Feb 19, 2022

Only the last commit is interesting - the others are in #47938.

At this point, isDefinition is wrong in ways that probably don't matter. The baselines show the different per-project FAR invocations and the product diff indicates the maximum simplification we can achieve (adding
code to fix up isDefinition can only make it worse).

I'm generally okay with the test change (seeing fewer results when a file is specified) both in principal, since not loading projects seems preferable when scoping to the active document, and in practice, since AFAIK only old version of VS Code pass a file with no project.
This change had two goals:

1. Make the code easier to understand, primarily by simplifying the callback structure and minimizing side-effects
2. Improve performance by reducing repeated work, both FAR searches of individual projects and default tsconfig searches

Most of the baseline changes just reflect the de-duping of tsconfig searches, a few show fewer and occasionally different FAR invocations, and a couple show response changes.  I'm quite confident that the new FAR calls are better and moderately confident that the new isDefinition values are better (not to mention moderately skeptical that anyone will ever hit a case where the difference matters).
At this point, `isDefinition` is wrong in ways that probably don't matter.  The baselines show the different per-project FAR invocations and the product diff indicates the maximum simplification we can achieve (adding code to fix up `isDefinition` can only make it worse).
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 19, 2022
@amcasey amcasey closed this Mar 3, 2022
@amcasey amcasey deleted the FarIsDefinition branch March 3, 2022 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants