search

onivim / oni

Oni: Modern Modal Editing - powered by Neovim
https://www.onivim.io
MIT License
11.36k stars 301 forks source link

Language Server Client: Typing +/- Refactor #2235

Open akinsho opened 6 years ago

akinsho commented 6 years ago

This is an entirely development related issue, in looking into #1717 I've been looking at the lsp client we have a lot and was thinking to add some typing but given that types depend on the protocol I've ended up reading through this and looking at this language-server-client example.

Although the file is thousands of lines long and possibly significantly more complex than oni's current requirements (scratch that definitely way more complicated). I thought looking through that implementation example oni could possibly benefit from more closely trying to mirror that.

The reasons for this are that the example is v. well typed which provides the benefit of making the inputs and outputs clearer to follow, it monitors the process(es) more closely making sure things are tidied up and seems to have a slightly easier to follow extensibility pattern

It could very well be that this might be too ambitious or unnecessary (or both) but I wanted to raise this issue before looking too closely at it. That being said it might be entirely beyond me because whilst some parts of that are v. clearly documented (which is also something I think it would be cool for a refactor to look to do as well) some sections are completely over my head, again to clarify I wouldn't be looking necessarily to add to our lsp in an initial PR just remodel it to look more like that then we might be able to use it as a reference point for further development.

Thoughts @bryphe @CrossR?

bryphe commented 6 years ago

Definitely open to improving our language client! Thanks for thinking about this, @Akin909 .

The reasons for this are that the example is v. well typed which provides the benefit of making the inputs and outputs clearer to follow

Hmm, can you give specific examples here? I know one difference is they use strongly typed events, whereas ours just has sendRequest(requestName: string, ...payload), sendNotification(requestName: string, ...payload), handleRequest(requestName: string, handlerFunction: () => { }). We could certainly refactor to use strongly-typed events (either on the LanguageClient object, or a layer on top). Is that what you were thinking of, or something else?

it monitors the process(es) more closely making sure things are tidied up

Do you have a specific example? Can you provide more details on what we can improve here, and specifics on how the language-server-client-example implements this improvement?

seems to have a slightly easier to follow extensibility pattern

Same as above, do you have a specific example you can point to? Or can you provide more details on a specific case?

In general, @Akin909 , could you break this down into a more specific, concrete set of items for us to refactor? Otherwise this issue isn't actionable in its current form - it's too vague and there's no clear completion criteria.

I'm definitely open to improving this layer, but I'd like to see a more concrete proposal.

akinsho commented 6 years ago

@bryphe

In general, @Akin909 , could you break this down into a more specific, concrete set of items for us to refactor? Otherwise this issue isn't actionable in its current form - it's too vague and there's no clear completion criteria.

I'm definitely open to improving this layer, but I'd like to see a more concrete proposal.

👍 Agreed this isn't v. concrete I was hoping to get a feel for where we were on possible changes to the current setup aka fine as is etc. but wasn't v. clear re. what I envisaged.

Firstly I think what I'd like to do is split this or rather close this issue and raise one by one over time more concrete issue relating to specific changes.

To start what you raise re. the strongly typed events I would says is a v. big part of exactly what I meant here aka an interface the client implements and then a series of strongly typed methods relating each bit of ls functionality. I think these can all be modularised as they are now but if each method is typed and the handler must match the typings then it'll allow for type checking but also serve as documentation for each bit of the lsp.

Re. the points on the process and extensibility I can link snippets to sections which I looked over which seem like they'd be useful to Oni but think what I'd rather do is taking things a step at a time focus on the above then look at detailed issue re specific sections of the example and how we might use bits of it later on.

If your in agreement I'll close this and raise a specific issue to the typing as I think there are a couple of examples or different sections of the repo or another of the example repos I was looking at the illustrate what they seem to be doing slightly better.