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

VSCODE-231: Catch completion error when not connected #251

Merged

Conversation

alenakhineika
Copy link
Contributor

@alenakhineika alenakhineika commented Feb 2, 2021

Description

  • Do not create a worker thread for the empty extension path.
  • Set extension path when starting the language server instead of doing it with each new connection.
  • The error described in the ticket was caused by both items above. The extension path was undefined because a user wasn't connected, and the error wasn't caught in the language server to just being logged there, but was propagated upper to the playground controller and therefore was shown to the user, what we would like to avoid.
  • Replace some any with proper types in the language server.
  • Clean up returning values to math declared types.
  • Bamp some dependencies to fix vulnerabilities.

https://jira.mongodb.org/browse/VSCODE-231

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@alenakhineika alenakhineika requested a review from Anemy February 2, 2021 18:33
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

nice, since we are updating a bunch of dependencies we manually test this a good bit before release. Especially playgrounds. Couple comments, not blockers, mostly on types and definitions.
Nice job adding more types and reducing any usage.

_cachedCollections: object;
_cachedShellSymbols: any;
_connectionOptions?: NodeOptions;
_cachedDatabases: CompletionItem[] | [];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can make this _cachedDatabases: CompletionItem[] = [] and remove the setting in the constructor. Same with _cachedFields and _cachedCollections

@@ -25,3 +26,29 @@ export type CloudInfoResult = {
isGcp: boolean;
isAzure: boolean;
};

export type NodeOptions = {
Copy link
Member

@Anemy Anemy Feb 3, 2021

Choose a reason for hiding this comment

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

I think we already have a place where it might make sense for this to live: https://github.com/mongodb-js/vscode/blob/master/src/connectionModelType.ts#L5

Kind of an aside, how do you feel about having a folder for these types, so instead of utils/types.ts it's types/CollectionItem.ts ortypes/ConnectionModel.ts.

With connection model maybe we can even go further and similar to how we treat .less files: https://github.com/mongodb-js/vscode/blob/master/src/views/webview-app/externals.ts but in this case we can define the mongodb-connection-model
we can create a type definition for that module and use that. Maybe that's a different part of work from this though we can do later. (Maybe we don't bother if we want to have our own connection model in this repo also).

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Nice
Are DocumentSource, InstanceInfoResult, and cloudInfoType only used for telemetry? maybe they file can exist in the telemetry folder?
Tried to repro the error - looks like its handled nicely now 👍

@alenakhineika
Copy link
Contributor Author

@Anemy I have noticed that when types are stored in separate folders with time some of them become needed for other modules, but you don't remember already that these types exist and duplicate them. when all types stored in one folder it is easier to navigate and manage them. but I am open to discussion and finding a better way to deal with it.

@alenakhineika alenakhineika merged commit 34eca98 into master Feb 5, 2021
@alenakhineika alenakhineika deleted the VSCODE-231-catch-completion-error-when-not-connected branch February 5, 2021 17:13
@Anemy Anemy mentioned this pull request Feb 10, 2021
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