Offer auto-imports from wildcard exports with AutoImportProvider #54831
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot user test tsserver |
|
Heya @andrewbranch, I've started to run the diff-based user code test suite (tsserver) on this PR at 2fc2d7d. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 2fc2d7d. You can monitor the build here. Update: The results are in! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@andrewbranch Here are the results of running the user test suite comparing Everything looks good! |
|
@andrewbranch Here they are:
CompilerComparison Report - main..54831
System
Hosts
Scenarios
TSServerComparison Report - main..54831
System
Hosts
Scenarios
StartupComparison Report - main..54831
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
The tsserver perf tests didn’t exercise the new code in a meaningful way, apparently (the completion counts were unchanged). I’ll try to do a little manual comparison. |
|
Discovered an unrelated bug in module specifier generation for wildcard+conditional exports that probably prevented any new completions from being found in the perf tests. Local testing shows that AutoImportProvider initialization time was barely impacted at all even though it finds a good bit more stuff, and then completions response time is pay-for-more-completions as expected. @typescript-bot perf test Unless we see anything weird in the updated perf test, I think this is fine. |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 04c06ee. You can monitor the build here. |
andrewbranch
changed the title
sandersn
approved these changes
Jun 30, 2023
|
Hey @andrewbranch, something went wrong when publishing results. (You can check the log here). |
|
@typescript-bot perf test |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 04c06ee. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here they are:
CompilerComparison Report - main..54831
System
Hosts
Scenarios
TSServerComparison Report - main..54831
System
Hosts
Scenarios
StartupComparison Report - main..54831
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -2620,7 +2622,7 @@ export class AutoImportProviderProject extends Project { | |||
| throw new Error("package.json changes should be notified on an AutoImportProvider's host project"); | |||
| } | |||
|
|
|||
| override getModuleResolutionHostForAutoImportProvider(): never { | |||
| override getHostForAutoImportProvider(): never { | |||
There was a problem hiding this comment.
Should this have been internal to begin with
There was a problem hiding this comment.
It’s never been clear to me what parts of TS Server are intended to be public API. It seems like this whole class could be internal.
I'm kind of curious what's meant by this because I personally found The naive implementation of "just return the whole directory listing for the given directory" (and the name implies that should be its only responsibility) ended up pulling too much into the compilation (i.e. literally everything in node_modules, recursively) |
|
Yeah, I mean it does so much. It’s based on |
jakebailey
reviewed
Jul 5, 2023
| @@ -194,6 +198,15 @@ function formatExtensions(extensions: Extensions) { | |||
| return result.join(", "); | |||
| } | |||
|
|
|||
| function extensionsToExtensionsArray(extensions: Extensions) { | |||
There was a problem hiding this comment.
Out of curiosity, how often does this happen? Is it worth caching in some way given the key is just a number?
There was a problem hiding this comment.
The number of packages AutoImportProvider will process is capped at 10, so surely not enough times to be make an observable impact.
| @@ -2140,7 +2158,7 @@ export function getEntrypointsFromPackageJsonInfo( | |||
| function loadEntrypointsFromExportMap( | |||
| scope: PackageJsonInfo, | |||
| exports: object, | |||
| state: ModuleResolutionState, | |||
| state: ModuleResolutionState & { host: GetPackageJsonEntrypointsHost }, | |||
There was a problem hiding this comment.
I was going to ask if this would be better served as a parameter rather than tacked onto the state, but it does seem like this was already happening given what's in useCaseSensitiveFileNames.
There was a problem hiding this comment.
This isn’t adding a host property, it’s narrowing the type of the existing host property.
There was a problem hiding this comment.
Yeah, I was just unsure without trying to find this type (which I should have done!)
sheetalkamat
approved these changes
Jul 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ts.sys.readDirectoryis bananas 🍌. I thought this was going to be harder.Fixes #53116