Skip to content

Conversation

alenakhineika
Copy link
Contributor

@alenakhineika alenakhineika commented Apr 4, 2020

Description

  • In this PR I added instantiation of Language Server to the mdbExtensionController and moved playground execution to this Language Server.

  • The playground makes a call to the Language Server Client and sends two parameters: code that needs to be evaluated and driverUrl.

  • The Language Server Client sends a request with these values to the Language Server Server.

  • Looks like we can't use DataService in the Language Server Server. Probably because it extends from EventEmitter. But we can use CliServiceProvider from '@mongosh/service-provider-server' instead. It accepts 2 params driverUrl and driverOptions. It provides similar methods as DataService does: https://github.com/mongodb-js/mongosh/blob/master/packages/service-provider-server/src/cli-service-provider.ts. Please let me know if you have any objections? The investigation ticket was created: https://jira.mongodb.org/browse/VSCODE-81

  • I have noticed that the "Extension + Server Inspector" and "Attach to Language Server" runners from Launch file do not work. The first thing it should be 6009 instead of 6005, but even if I change it, Vscode complains anyway that a connection is refused.

  • Moved code from the activate method of the Language Server to the constructor in order to set this.client value in the constructor and get rid of later checks on undefined in each method with sendRequest.

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
Copy link
Contributor Author

Hey @imlucas, after our whiteboarding meeting, I have realized, that I need to try it right away, instead of getting deeper into theory. It will help me to better understand the concept. This is how I see the moving of playground to the language server. And what do you think about using CliServiceProvider instead of DataService? Looking forward to your feedback.

@alenakhineika alenakhineika requested review from Anemy and imlucas April 4, 2020 17:32
@alenakhineika alenakhineika force-pushed the VSCODE-79-move-playground-to-language-server branch from faa4460 to b4ce48e Compare April 5, 2020 10:54
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.

Awesome. One comment on error messaging, and one on infinite loops then should be good to go.

This runs smooth on my local - I think using the cli service provider is fine, I don't think there should be any limitations?

I think one of the benefits of running on a separate process is protection from infinite loops. Should we look at how we can handle those in a new ticket? Currently if the server gets locked up it's kind of silently failing. I'm thinking maybe we can have a series of messaging when executing playgrounds to ensure first that the server is ready to run a command before trying to run it. That way we can timeout or cancel a playground if the server is already locked up.

I have noticed that the "Extension + Server Inspector" and "Attach to Language Server" runners from Launch file do not work.

Anything we should do here? I haven't used that feature before.

/**
* Opens the MongoDB playground
*/
export async function openPlayground(docUri: vscode.Uri) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this used elsewhere - or can we remove export (so lint can pick up if we stop using it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing it!

await CliServiceProvider.connect(params.connectionString, connectionOptions)
);

return await runtime.evaluate(params.codeToEvaluate);
Copy link
Member

@Anemy Anemy Apr 6, 2020

Choose a reason for hiding this comment

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

I think we could move the evaluate call into a separate try from the connection that way a code error can be better handled. Currently if there's an error in a playground's code, like calling a method that doesn't exist the error message is Unable to connect to MongoDB instance. Make sure it is started correctly.. Does the runtime throw an error that we can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed try-catch at all, since they are being caught in the playground controller anyway.

@alenakhineika
Copy link
Contributor Author

Created a ticket for cancellation token: https://jira.mongodb.org/browse/VSCODE-82

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.

lgtm. This is a good step to getting some functionality using a language server. Thanks for creating that ticket on interrupting the server on a long task - I think we can get a good user experience out of it.

@alenakhineika alenakhineika merged commit 0082024 into master Apr 6, 2020
@alenakhineika alenakhineika deleted the VSCODE-79-move-playground-to-language-server branch April 6, 2020 15:34
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.

3 participants