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

Let VSCode start the language server #4304

Merged
merged 9 commits into from Sep 14, 2018

Conversation

Projects
None yet
4 participants
@Duhemm
Copy link
Contributor

commented Apr 12, 2018

We used to start the language server with launchIDE. However, this
means that the language server is dependent of sbt being running and
configured, which is not a good user experience for newcomers.

For newcomers, we want the language server to start when they create a
new Scala file in an empty directory, for instance, which means that no
build is already configured.

This commits takes us closer to this experience by using load-plugin
to inject the Dotty plugin. This means that the Dotty plugin can now be
loaded even if there's no build configured, which happens if the user
opens VSCode in an empty directory, for instance.

We use the recently introduced --addPluginSbtFile to provide this experience.

@@ -17,12 +17,9 @@ export function activate(context: ExtensionContext) {
outputChannel = vscode.window.createOutputChannel('Dotty Language Client');

const artifactFile = `${vscode.workspace.rootPath}/.dotty-ide-artifact`
const defaultArtifact = "ch.epfl.lamp:dotty-language-server_0.8:0.8.0-bin-SNAPSHOT"

This comment has been minimized.

Copy link
@Duhemm

Duhemm Apr 12, 2018

Author Contributor

If this get in, we'd need to find a way to keep these values up-to-date with the reality. When's the vscode extension published? Every 6 weeks, too?

This comment has been minimized.

Copy link
@smarter

smarter Apr 12, 2018

Member

You can get the current nightly base of dotty from http://dotty.epfl.ch/versions/latest-nightly-base, we could also have a file to get the current release version.

configureIDE().then((res) => {
run({
command: "java",
args: ["-classpath", classPath, "dotty.tools.languageserver.Main", "-stdio"]

This comment has been minimized.

Copy link
@Duhemm

Duhemm Apr 12, 2018

Author Contributor

Any reason why we extract the classpath with coursier fetch and then run this way rather than doing coursier launch?

This comment has been minimized.

Copy link
@smarter

smarter Apr 12, 2018

Member

We display a progress notification while coursier fetch is running, if we use coursier launch we don't know when downloading has finished.

@@ -24,6 +24,7 @@
],
"main": "./out/src/extension",
"activationEvents": [
"onLanguage:scala",

This comment has been minimized.

Copy link
@smarter

smarter Apr 12, 2018

Member

The problem with doing this is that we conflict with every other language server for Scala

This comment has been minimized.

Copy link
@Duhemm

Duhemm May 11, 2018

Author Contributor

What can we do instead? The goal of this PR is to be able to get the Dotty IDE working without any prior setup (so, nobody would have generated .dotty-ide.json).

This comment has been minimized.

Copy link
@Duhemm

Duhemm Aug 17, 2018

Author Contributor

Now, if there's no configuration for the IDE a message is shown asking the user whether she wants to start the IDE or not. If there's a conflict with another language server, she can just say no. If the IDE is configured, then it'll automatically start up.

@@ -51,7 +51,7 @@
"vscode:prepublish": "npm install && ./node_modules/.bin/tsc -p ./",
"compile": "./node_modules/.bin/tsc -p ./",
"test": "node ./node_modules/vscode/bin/test",
"postinstall": "node ./node_modules/vscode/bin/install && curl -L -o out/coursier https://github.com/coursier/coursier/raw/v1.0.0/coursier && curl -L -o out/load-plugin.jar https://github.com/scalacenter/load-plugin/releases/download/v0.1.0/load-plugin_2.12-0.1.0.jar"
"postinstall": "node ./node_modules/vscode/bin/install && curl -L -o out/coursier https://github.com/coursier/coursier/raw/v1.0.0/coursier && curl -L -o out/load-plugin.jar https://oss.sonatype.org/content/repositories/releases/ch/epfl/scala/load-plugin_2.12/0.1.0+2-496ac670/load-plugin_2.12-0.1.0+2-496ac670.jar"

This comment has been minimized.

Copy link
@smarter

smarter Apr 12, 2018

Member

Can't you use coursier to fetch the load-plugins jar?


sbtProc.on('close', (code: number) => {
if (code != 0) {
let msg = "Configuring the IDE failed."

This comment has been minimized.

Copy link
@allanrenucci

allanrenucci Apr 12, 2018

Member

const msg?

@laughedelic

This comment has been minimized.

Copy link

commented Apr 12, 2018

It's cool to see load-plugin in action 👏 But I expected that it happens in the server:

  1. user opens a file in the client (editor)
  2. client launches server (either reading from .dotty-ide-artifact or using some default)
  3. server checks if .dotty-ide.json is in place, if not runs load-plugin and configureIDE

This way client implementation stays minimal: it only needs to launch server using coursier. And this is important IMO because other clients may or may not reimplement the same thing: users experience will vary. If server takes care of its own automatic setup, then clients are simple and users get same awesome experience regardless of their editor of choice.

P.S. I don't know any details about Dotty LS so probably there's a good reason for doing it this way and what I say is completely invalid.

@Duhemm Duhemm force-pushed the dotty-staging:topic/start-lsp-from-vscode branch from 6fd30e8 to 02ce769 May 14, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

Hi @laughedelic!

I agree that this could be pushed to the server implementation rather than the client, but I think it has been done this way for two reasons:

  • Make the client more interactive (for instance, show that it is downloading stuff). I'm not sure how this could be done if the server was the one in charge of downloading sbt, and load-plugin, but LSP may very well have something for that...
  • Have fewer dependencies in the LSP server. I think that coursier comes with a ton of dependencies, unfortunately.

I'd be open to try the approach that you suggest. Having the smallest client possible is definitely a big bonus. I think that @smarter may be able to shed more light on why the client works the way it does.

@Duhemm Duhemm added stat:needs review and removed stat:wip labels May 14, 2018

@Duhemm Duhemm changed the title [WIP] Let VSCode start the language server Let VSCode start the language server May 14, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

@smarter I think I've addressed your comments, apart from the one regarding the activationEvents, because I don't know what to do.

The goal is to be able to start the Dotty LSP and automatically configure it when the user opens a .scala file, so I don't see any way around the conflict with other LSP servers.

@smarter

This comment has been minimized.

Copy link
Member

commented May 14, 2018

First we need to actually test how language server interact in VSCode. What happens when I have both one of the Scala 2 LS and the Dotty LS with this PR enabled at the same time, in a Scala 2 project? The Dotty LS shouldn't be doing anything since the sbt plugin won't find any Dotty project, so it's possible that things "just work". If they don't, it might help to actually shut down the Dotty LS if no Dotty project has been found, that way only one LS would run at a given time for a given project.

@Duhemm Duhemm force-pushed the dotty-staging:topic/start-lsp-from-vscode branch from 02ce769 to 462e850 Jun 11, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

I changed it so that the extension will ask the user whether the IDE should be started when it detects that there's no configuration (no .dotty-ide.json, no build.sbt) :

screen shot 2018-06-11 at 14 27 35

@Duhemm Duhemm force-pushed the dotty-staging:topic/start-lsp-from-vscode branch from 462e850 to cfcacc1 Aug 17, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

I updated the PR to use --addPluginSbtFile.

@Duhemm Duhemm assigned smarter and unassigned Duhemm Sep 4, 2018

runLanguageServer(coursierPath, languageServerArtifactFile)
} else if (!fs.existsSync(buildSbtFile)) {
vscode.window.showInformationMessage(
"This looks like an unconfigured project. Would you like to start Dotty IDE?",

This comment has been minimized.

Copy link
@smarter

smarter Sep 14, 2018

Member

"an unconfigured Scala project", "start the Dotty IDE"

Also maybe remember the choice of the user by creating an empty file ".dotty-ide-disabled" ?

if (err) {
outputChannel.append(`Unable to parse ${portFile}`)
throw err
const sbtArtifact = "org.scala-sbt:sbt-launch:1.2.0"

This comment has been minimized.

Copy link
@smarter

smarter Sep 14, 2018

Member

Upgrade to 1.2.3

function configureIDE(sbtClasspath: string, languageServerScalaVersion: string, dottyPluginSbtFile: string) {
return vscode.window.withProgress({
location: vscode.ProgressLocation.Window,
title: 'Configuring IDE...'

This comment has been minimized.

Copy link
@smarter

smarter Sep 14, 2018

Member

"Configuring the IDE for Dotty"

val packageJson = workingDir / "package.json"
// val _ = (managedResources in Compile).value

This comment has been minimized.

Copy link
@smarter

smarter Sep 14, 2018

Member

Can be deleted.

Duhemm added some commits Apr 9, 2018

Let VSCode start the language server
We used to start the language server with `launchIDE`. However, this
means that the language server is dependent of sbt being running and
configured, which is not a good user experience for newcomers.

For newcomers, we want the language server to start when they create a
new Scala file in an empty directory, for instance, which means that no
build is already configured.

This commits takes us closer to this experience by using `load-plugin`
to inject the Dotty plugin. This means that the Dotty plugin can now be
loaded even if there's no build configured, which happens if the user
opens VSCode in an empty directory, for instance.
Update `load-plugin`
Use `if-absent` so that we don't load the plugins and apply
transformations to the build if this is not necessary.
Retrieve `load-plugin` with coursier
And retrieve everything (sbt, dotty-language-server, load-plugin) in
parallel.

Duhemm added some commits Jun 11, 2018

Show message to start the IDE
When the extension detects that this is an un-configured project (no
`.dotty-ide-artifact`, no `build.sbt`), it shows a message asking the
user whether she wants the IDE to be started.

@Duhemm Duhemm force-pushed the dotty-staging:topic/start-lsp-from-vscode branch from cfcacc1 to 8a48286 Sep 14, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2018

@smarter Comments addressed.

@smarter

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

OK, feel free to merge.

@Duhemm Duhemm merged commit 0a2734b into lampepfl:master Sep 14, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@Duhemm Duhemm deleted the dotty-staging:topic/start-lsp-from-vscode branch Sep 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.