FurqanSoftware / codemirror-languageserver

Language Server integration for CodeMirror 6
BSD 3-Clause "New" or "Revised" License
183 stars 24 forks source link

feat: Transport agnostic #13

Closed marc2332 closed 2 years ago

marc2332 commented 2 years ago

Reference: https://github.com/FurqanSoftware/codemirror-languageserver/issues/12

It now accepts any object implementing Transport.

I removed processRequest method which wasn't needed btw.

The example now would look something like:

import { WebSocketTransport } from "@open-rpc/client-js";
import { languageServer } from 'codemirror-languageserver';

// WebSocket transport
const transport = new WebSocketTransport(serverUri)

var ls = languageServer({
    // Transport and other client options.
    transport,
    rootUri: 'file:///',

    // Alternatively, to share the same client across multiple instances of this plugin.
    client: new LanguageServerClient({
        transport,
        rootUri: 'file:///'
    }),

    documentUri: `file:///${filename}`,
    languageId: 'cpp' // As defined at https://microsoft.github.io/language-server-protocol/specification#textDocumentItem.
});

var view = new EditorView({
    state: EditorState.create({
        extensions: [
            // ...
            ls,
            // ...
        ]
    })
});

I have tried it with Event Emitter and WebSockets transports and both work fine! :-)

image

Awesome job you have done with this library

marc2332 commented 2 years ago

Maybe an optional function could be made just to avoid having the user write more code. This new function would ultimately call languageServer internally passing a WebSocket transport.

hjr265 commented 2 years ago

@marc2332 Amazing! Thank you so much for this.

I just have one request. I think it is possible to introduce this change without having others to update their usage of this library?

So potentially, we keep this same function but make it take either "serverUri" or "transport". We give "transport" more priority. So if it is set, we use that. Otherwise, we use "serverUri" to make our a WebSocket transport internally and use that.

We may eventually deprecate the "serverUri" option in the future. But for now, we accommodate this new feature without any breaking changes.

Would you be open to making this change to this PR?

marc2332 commented 2 years ago

Yeah, I had the same thought on my previous comment. Maybe we could leave languageServer() accepting serverUri (and therefore using the WebSockets transport) and create an optional function which would let pass a custom transport. Does it sound good? If not we can do what you say about having the transport option but giving serverUri more priority

hjr265 commented 2 years ago

@marc2332 So you mean having languageServer(...) and languageServerTransport(...)?

Sure... As long as we don't have to bump the major version of this project. :slightly_smiling_face:

I suppose, internally, languageServer can then become a wrapper of languageServerTransport.

marc2332 commented 2 years ago

@marc2332 So you mean having languageServer(...) and languageServerTransport(...)?

Sure... As long as we don't have to bump the major version of this project. slightly_smiling_face

I suppose, internally, languageServer can then become a wrapper of languageServerTransport.

Yeah, something like that! Yep, languageServer would simply call languageServerTransport passing the WebSocket transport.

That will avoid having breaking changes :-)

marc2332 commented 2 years ago

I'll work on it asap :)

hjr265 commented 2 years ago

@marc2332 Although, on second thought, by splitting the function, you will still have to keep the serverUri and transport property defined on both the LanguageServerClientOptions and LanguageServerOptions objects. Or, you will have to define a third options objects.

I am wondering how that will pan out.

marc2332 commented 2 years ago

@marc2332 Although, on second thought, by splitting the function, you will still have to keep the serverUri and transport property defined on both the LanguageServerClientOptions and LanguageServerOptions objects. Or, you will have to define a third options objects.

I am wondering how that will pan out.

That can be improved by having a base interface with the required fields such as rootUri, documentUri and languageId, and then 2 interfaces extending this one, one with the serverUri and the other with the tranport :-)

hjr265 commented 2 years ago

@marc2332 You can guess I am new to TypeScript. :slightly_smiling_face:

That sounds good.

marc2332 commented 2 years ago

Okay, so I have added languageServerWithTransport(), languageServer() has the previous signature (no breaking changes :))

I have also updated dependencies (I was having some duplicated @codemirror/state error). Anyway, turns out codemirror/tooltip has been merged into codemirror/view !

hjr265 commented 2 years ago

@marc2332 Nice stuff! Thank you again for doing this.

Sorry it took me a while to actually get my project's CodeMirror dependencies updated and then have this tested there.

marc2332 commented 2 years ago

@marc2332 Nice stuff! Thank you again for doing this.

Sorry it took me a while to actually get my project's CodeMirror dependencies updated and then have this tested there.

No problem, thank you! I am looking forward to contributing more, this is library is really useful for me :-))

hjr265 commented 2 years ago

@marc2332 Looking forward to it. Glad to know you found this library useful! :slightly_smiling_face: