-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Add 'data' property to completion entry for better coordination between completions and completion details #42890
Conversation
…n completions and completion details
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 |
@andrewbranch As I work on the implementation for this, I'm hitting a couple issues:
|
@uniqueiniquity the second of those problems should be fixed. The first I can’t reproduce with a server fourslash test, so we might have to debug together at some point. |
As discussed offline, the first problem came from a TSServer plugin that patches |
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.
I don't know enough to comment on the high-level concerns, but I looked over the code itself and only had one comment.
}); | ||
} | ||
} | ||
} | ||
|
||
function getAutoImportSymbolFromCompletionEntryData(data: CompletionEntryData): { symbol: Symbol, origin: SymbolOriginInfoExport } | undefined { | ||
const containingProgram = data.isPackageJsonImport ? host.getPackageJsonAutoImportProvider!()! : program; |
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.
I think given that this is passed around we need to handle cases where data doesnt match what was passed around so instead of asserting we probably want to ensure that we handle if things are absent
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.
I don’t think that would make much difference—we already have an assertion later that getCompletionDetails
can successfully find the completion by entryId
. Even in master, if a bad client passed an incorrect name
and source
combination, we would trigger that assertion. We could easily return undefined
here, but an assertion would be triggered a little bit later when we couldn’t find the module in question. I could replace some of this with a Debug.checkDefined
but I think it’s generally better to fail fast if data
violates the assumptions we hold.
Fixes #42871.