lsp4s/jsonrpc: minimize dependencies

jastice commented 6 years ago

I'm currently working on experimental support for Build Server Protocol in the IntelliJ Scala plugin. Since the bsp API uses lsp4s, I am pulling in all its dependencies, which amount to about 34MB. I am hesitant about pulling in this much since it would add a big chunk onto the Scala plugin's artifact size.

Logback and groovy seem like avoidable dependencies at the very least. A large bulk is also coming in via cats, but that is depended on by several other dependencies.

jastice commented 6 years ago

cc @jvican

gabro commented 6 years ago

There was some prior (but, I'm afraid, private) discussion on this matter before.

For example, using multiple libraries that depend on cats was an explicit choice for paying that price only once.

Let's try to analyze what are the low hanging fruits, if any, and if not we may decide to take some more drastic actions, in case this becomes blocking.

olafurpg commented 6 years ago

I agree! I looked into the classpath the other day and was ashamed how much redundant stuff was there 🙈 I think the best long term strategy here is to extend lsp4j (https://github.com/eclipse/lsp4j) to include bsp data structures. This will be needed for the Dotty language server either way.

We can totally slim the classpath

logback/groovy is most redundant, it came from the old lsp implementation from https://github.com/dragos/dragos-vscode-scala A PrintStream would do.
monix is there because we're already using it in metals. We use a tiny fraction of the API in jsonrpc so in theory we could provide a facade that can be implemented via stdlib Future if necessary.
circe was chosen for json but it could be replaced with facade traits for serialize/deserialize.
cats comes via circe and upcoming v3 of monix.
the LanguageServer and LanguageClient implementations could be moved to jsonrpc, they're a bit tied to lsp but jsonrpc is kind of useless without them.

That could make a 0-dependency package which you could plug in with whatever json/future/logging infrastructure you already have available. Question if you prefer over a bsp4j library that extends https://github.com/eclipse/lsp4j

olafurpg commented 6 years ago

The total amount of code in lsp4s+jsonrpc is ~1200 loc, of which ~400 are lsp case classes. It's a small library that should be fairly easy to refactor. The primary goal was to get something usable for metals so I used mostly what we had available already.

gabro commented 6 years ago

Ah, yes, thanks for clarifying a few things @olafurpg. When I said

For example, using multiple libraries that depend on cats was an explicit choice for paying that price only once.

I got things mixed up: this is not true for jsonrpc/lsp4s, they could do without circe/cats I think.

jastice commented 6 years ago

For my purposes a dependency-less version is ideal, but I'd be ok with pulling in cats and monix since they are general-purpose enough to be used in other parts of the plugin. logback and groovy not so much :)

jvican commented 6 years ago

It sounds like a good first step would be to remove the circe and cats dependency (assuming Monix stays in 2.x) and logback (which brings in groovy). Then, in the future, we could make everything dependency-free and potentially remove the monix dependency too. I'm afraid doing everything at once would take too much time for the kind of benefit we would get in return.

olafurpg commented 6 years ago

Keeping monix v2 would be the simplest for starters, the rest is fairly easy to take out. One avenue worth exploring might be to implement jsonrpc in terms of http://www.reactive-streams.org/ interfaces. Monix data structures already implement those interfaces so we could keep monix in metals while allowing other jsonrpc users to bring in their own favorite reactive streams implementation (which iiuc includes Flow that comes baked in with JDK 9).

olafurpg commented 6 years ago

I took a look at the lsp4s code and although it's possible to abstract out the json dependency, I think it would come at the price of usability (you'd have to bring your own json library) and maintainability (the core is no longer self-contained, code is harder to follow). I would prefer to settle on a concrete json library that is lightweight enough. The smallest alternative that still provides parser + printer + manual JSON AST manipulaton + automatic case class mapping I'm aware of is upickle at ~500kb, any suggestions?

gabro commented 6 years ago

I haven't looked at the code in a long time, would this mean to switch json library also in metals?

olafurpg commented 6 years ago

It wouldn't have to mean switching libs in metals, but most likely yes. This is a scenario where having something in the stdlib would be helpful in making the choice for you.

jvican commented 6 years ago

Is using Scala JSON AST a possibility here?

olafurpg commented 6 years ago

JSON AST is not sufficient since lsp4s needs a parser + printer + automatic case class mapping (ideally with macro annotation).

After discussions from ScalaSphere, the idea came up to try and proguard and shade the lsp4s dependencies. Given that the module uses a tiny surface area of the monix/cats APIs it might be sufficient to keep the code mostly unchanged without imposing such a heavy dependency on end-users.

olafurpg commented 6 years ago

Me and @jvican discussed about moving lsp4s to another repo lsp4s/lsp4s. @jvican will take care of minimizing the dependencies, in particular I think the lowest hanging fruits are

remove logback altogether. Either pass in print stream or implement a simple API like

trait JsonRpcLogger {
def info(msg: => String): Unit
def warning(msg: => String): Unit
def error(msg: => String): Unit
}

use upickle for JSON to replace scalapb/circe/cats. We used https://github.com/scalapb-json/scalapb-circe because we already had those projects in metals and there was a protobuf schema for BSP messages.

That should remove all dependencies except Monix. I'd recommend to keep Monix since the Task[T] and Observable[T] abstractions are a great fit for the event-driven and bi-directional LSP/BSP protocols, especially thanks to the first-class support for cancellation in Monix.

gabro commented 6 years ago

Thank you @jvican for taking this on! I'm happy to see lsp4s growing its own legs and moving to a different repo. If two LSP/BSP servers use it, it's already a big win for reusable components in the tooling space!

Let me know if you need help with some git-foo for preserving the commit history and attributions (I've amassed quite a bit of experience in git repo surgery at work)

jvican commented 6 years ago

@gabro It would be invaluable if you help me get started and migrate the current repo to the new repo. I literally have no idea how to do that.

@olafurpg Your plan sounds good, roughly what I had in mind!

I'm happy to see lsp4s usage growing up. It's an excellent library.

gabro commented 6 years ago

Sure, I'll do it. Something like

git filter-branch --tree-filter "find . -type f -not \( -wholename './lsp4s/*' -o -wholename './build.sbt' \) -delete" HEAD

should do the trick 😜

gabro commented 6 years ago

Surgery completed, the patient seems fine, take a look at https://github.com/lsp4s/lsp4s and try to see whether anything seems strange

/cc @olafurpg @laughedelic @jvican

laughedelic commented 6 years ago

Hooray! 🎉

Why lsp4s org? Will it contain any other repos? Also the license file is missing. Apart from that, it looks fine and I'm really glad to see it in its own repository! 👍

gabro commented 6 years ago

Why lsp4s org? Will it contain any other repos?

It was an easy default choice, in lack of another obvious org where to put it.

olafurpg commented 6 years ago

lsp4s doesn't depend on any scala.meta._ components, it's a self-contained library unrelated to metaprogramming in Scala.

gabro commented 6 years ago

Once we're happy with the migration and we're able to publish artifacts, we can remove it from this repo.

laughedelic commented 6 years ago

What about scalacenter org? or just some general org that could accommodate later some other related repos, like scala-tooling or scala-lsp?

gabro commented 6 years ago

What about scalacenter org?

We've considered it, but since this is not an official scalacenter project, so it would've been a bit strange.

just some general org that could accommodate later some other related repos, like scala-tooling or scala-lsp

That would take time for people to agree on. If that happens, I will happily transfer the repo there :) For the time being, I think the lsp4s org is a safe choice that gets us started.

laughedelic commented 6 years ago

OK! thanks for clarifying.

jastice commented 6 years ago

https://github.com/lsp4s/lsp4s is 404 now, where did it go? :)

olafurpg commented 6 years ago

It moved to https://github.com/scalameta/lsp4s :)

CI is configured, but will need more time to setup release and also pull in tests from a branch I have somewhere floating.

olafurpg commented 6 years ago

I have a WIP branch in https://github.com/scalameta/lsp4s/compare/master...olafurpg:json?expand=1 that refactors jsonrpc to remove logback + groovy + circe + enumeratum + cats bringing the total size down to ~3.5mb, of which 2.8mb is from monix and ~0.5mb is upickle.

I'd like to keep monix if possible, although I suspect it might be possible to keep only the reactive streams interfaces which are 4kb.

CI release is configured but I'd like to add a test suite to demonstrate how to unit test client/server interactions.

laughedelic commented 6 years ago

Wow! Impressive! 👏👏👏

rorygraves commented 6 years ago

Why do I feel like I am always a week behind in interesting discussions? - good stuff!

olafurpg commented 6 years ago

Cleanup of the jsonrpc and lsp packages is mostly complete https://github.com/scalameta/lsp4s/pull/4 Comments are welcome, I would like to commit to a stable source/binary API after this so now is a good time to bring up general UX issues

The test suite is definitely still lacking.

jastice commented 6 years ago

Closing and continuing at https://github.com/scalameta/lsp4s/issues/8

scalameta / metals

lsp4s/jsonrpc: minimize dependencies #285