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

Return null when resolving completion request at an invalid location #81

Conversation

keyboardDrummer
Copy link
Contributor

@keyboardDrummer keyboardDrummer commented Sep 27, 2018

Return null when resolving completion request at an invalid location, as is allowed by the LSP spec. Before this CR, asking for a completion request at a location where no completions can be provided, such immediately after the b in the code function b, would cause the server to return an ErrorCodes.InternalError.

The reason for this is that tsserver, when you ask for completions at a nonsense location, returns a response with success === false (ref). We then reject the tsp request Promise (here), and this rejected Promise is never caught so it is later translated to an internal error.

@gitpod-io-legacy-app
Copy link

Open in Gitpod - starts a development workspace for this pull request in code review mode and opens it in a browser IDE.

@keyboardDrummer keyboardDrummer force-pushed the typeFoxInvalidCompletionFix branch from 5335b57 to b925824 Compare September 27, 2018 12:39
return body.map(entry => asCompletionItem(entry, file, params.position, document));
} catch (error) {
if (error.message === "No content available.") {
this.logger.warn('No content was available for completion request');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to log? Clients will see them in the output panel, they are more noise than helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, could be useful for debugging. Do you prefer we make it this.logger.info ?

Copy link
Contributor

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

Check that there are no internal errors anymore, but instead of them, there are warnings now.

@akosyakov akosyakov merged commit fbefe3f into typescript-language-server:master Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants