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

vscode web extension #1300

Merged
merged 9 commits into from May 31, 2022
Merged

vscode web extension #1300

merged 9 commits into from May 31, 2022

Conversation

ogoffart
Copy link
Member

  • builds the lsp in wasm (this implies building as a library for wasm)
  • the wasm lsp cannot use the lsp-server crate to communicate because that uses stdin/stdout, and we need to communicate with the js runtime. So that's why the code there is a bit different.
  • The preview is not yet implemented
  • Currently only file that are open in the editor can be imported

@ogoffart ogoffart requested a review from tronical May 31, 2022 10:39
ogoffart and others added 6 commits May 31, 2022 12:42
This refactor the LSP to be possible to compile for wasm.
When using wasm we can't use the lsp-server crate because that one use the
stdin/stdout to communicate.
Instead, we need will do the communication in TypeScript using the vscode
language server protocol library, and serialize the types to wasm. Fortunately
that's easy because the lsp-types crate contains all the serialized types.

This also "duplicate" the extension as a web extension that do not use process
to start the LSP, but use a web worker. Some of the extension code could
be refactored to avoid some duplication (like the status bar handling and such).
And add a "browser" entry point in the package.json

Finally, add a browserServerMain.ts entry point for our worker, it will try to load
the wasm code.

Currently this doesn't wirk: the browserServerMain.ts can't load the wasm.

Also todo is to write the code so that the wasm code can send the response and
notifications.

To debug, I type these commands in editor/vscode directory

    npm run compile-web
    code --extensionDevelopmentKind=web --extensionDevelopmentPath=$PWD   ../..
* Use wasm-pack's web mode to create a .wasm file and an ESM module for the
  glue code
* Import the .wasm file directly in browserServerMain.ts
* And process it all with esbuild, which succeeds in seeing the .wasm import in
  browserServerMain.ts and then bundles it inline.
we need to use commonjs for the extension

If we start the client before the server has loaded the wasm, we don't
listen yet to the init message, so make sure not to start the client
before we sent the "OK" message back.

But it's still not working, the wasm code is called, but the parameter are
not properly passed
Now it calls in the wasm properly.
But i get borrowmut error because the wasm re-enter from send_notification into the request
Copy link
Member

@tronical tronical left a comment

Choose a reason for hiding this comment

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

Terrific, can't wait to try it. I can't see any harm in merging this already.

Comment on lines +31 to +33
if (m.data === "OK") {

client = new LanguageClient('slint-lsp', 'Slint LSP', clientOptions, worker);
Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I guess the LanguageClient will overwrite worker.onmessage?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.

#[wasm_bindgen]
pub fn handle_request(
&self,
_id: JsValue,
Copy link
Member

Choose a reason for hiding this comment

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

What is the role of the id? Should it be used or can it be omitted?

Copy link
Member Author

Choose a reason for hiding this comment

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

The id is indeed the token that can be used if we want to cancel the request in the future.

but this is not used right now.

});

connection.onRequest((method, params, token) => {
return the_lsp.handle_request(token, method, params);
Copy link
Member

Choose a reason for hiding this comment

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

Ah, here the token becomes the id. Should it be used?

@@ -88,6 +89,7 @@
"scripts": {
"vscode:prepublish": "npm run compile",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this helps with the CI:

Suggested change
"vscode:prepublish": "npm run compile",
"vscode:prepublish": "npm run compile && npm run compile-web",

@ogoffart ogoffart merged commit 8413480 into master May 31, 2022
@ogoffart ogoffart deleted the olivier/wip/lsp branch May 31, 2022 15:45
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.

None yet

2 participants