The Wayback Machine - https://web.archive.org/web/20211030082052/https://github.com/dotnet/interactive/pull/236
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

add stubs for lsp over http #236

Merged
merged 7 commits into from Mar 16, 2020
Merged

add stubs for lsp over http #236

merged 7 commits into from Mar 16, 2020

Conversation

Labels
None yet
3 participants
@brettfo
Copy link
Member

@brettfo brettfo commented Mar 4, 2020

This adds all of the stubs necessary to invoke the textDocument/hover LSP function, which is analogous to QuickInfo in Visual Studio. Currently only the C# kernel responds to this and it's responding with a hard-coded string. The work to produce the real content is forthcoming, but I needed the plumbing, first.

The LSP work is mostly split between two files. The first is lsp.js which handles the LSP-over-HTTP business. Not much to look at here.

The next part is specific to CodeMirror and how it's exposed through jupyter notebook. I've limited this PR to that case because it's simpler because any given web page has only one notebook open. That works by detecting the global JavaScript variables CodeMirror and Jupyter (not set in jupyter lab). Since CodeMirror doesn't natively understand a text hover operation, I've tied into CodeMirror cell creation and hook into mousemove and mouseleave to register timer-based callbacks that ultimately result in textDocument/hover being called if the mouse is in the same position for 500ms.

CodeMirror also doesn't have the ability to get the line/column of the character under the mouse cursor, so I've had to get creative. I do that by first iterating over the DOM elements that represent the lines of the editor. If one of those lines' bounding rectangles contains the mouse cursor, then I start to look for the column. Finding the column is much harder because the DOM elements don't represent anything useful, so I simply check each one in order, again checking the bounding rectangle. The end result is that the column that I detect is really the column of the first character of an arbitrary text span, but since CodeMirror is splitting the text on reasonable word boundaries, this will always yield the start position of what any programmer would call an identifier.

In the example below, the following markdown is returned as a hard-coded string from the C# LSP provider:

textDocument/hover at position (0, 9) with `markdown`

image

@colombod I re-worked the requirejs.config() call in HttpApiBootstrapper.cs so that the prefix dotnet-interactive points to http://localhost:<port>/resources. This makes it so that I can later call the following nicely:

require(['dotnet-interactive/dotnet-interactive'], ...); // returns http://localhost:<port>/resources/dotnet-interactive.js
require(['dotnet-interactive/lsp'], ...); // returns http://localhost:<port>/resources/lsp.js
require(['dotnet-interactive/code-mirror-lsp'], ...); // returns http://localhost:<port>/resources/code-mirror-lsp.js
@brettfo brettfo requested review from colombod and jonsequitur Mar 4, 2020
@brettfo brettfo force-pushed the httlsp branch 6 times, most recently from 448cecb to f435073 Mar 6, 2020
Copy link
Member

@colombod colombod left a comment

The main concern I have is with your rename from requirejs to require that caused few issues in jupyterlab (where that code is executed) and the removal of process+port cache buster in the require scope

Microsoft.DotNet.Interactive.Tests/Language.cs Outdated Show resolved Hide resolved
@@ -167,6 +168,11 @@ command switch

public abstract bool TryGetVariable(string name, out object value);

public virtual Task<JObject> LspMethod(string methodName, JObject request)
Copy link
Member

@colombod colombod Mar 6, 2020

are we expecting the api to be opt in or otp out? @jonsequitur should this be abstract?

Copy link
Member Author

@brettfo brettfo Mar 6, 2020

Ultimately I don't care either way. I made the method virtual simply so F#/pwsh don't have to implement this method simply to return null if they don't support LSP.

Copy link
Contributor

@jonsequitur jonsequitur Mar 12, 2020

JObject isn't an ideal return type. We should use made-for-purpose types here and let serialization happen higher up.

Copy link
Member Author

@brettfo brettfo Mar 13, 2020

I just added a commit titled strongly type LSP responses that does just this; take a look.

dotnet-interactive/HttpApiBootstrapper.cs Outdated Show resolved Hide resolved
dotnet-interactive/HttpApiBootstrapper.cs Outdated Show resolved Hide resolved
dotnet-interactive/HttpApiBootstrapper.cs Outdated Show resolved Hide resolved
dotnet-interactive/resources/code-mirror-lsp.js Outdated Show resolved Hide resolved
dotnet-interactive/resources/dotnet-interactive.js Outdated Show resolved Hide resolved
@brettfo
Copy link
Member Author

@brettfo brettfo commented Mar 12, 2020

Re-did this PR to take in recent changes. In short:

  1. Editor sniffing is now one layer up and it only loads the CodeMirror component if necessary.
  2. The element that is detected as the notebook root is decorated in two ways:
    A. The class dotnet-interactive-notebook-root is added.
    B. The key/value pair dotnet-interactive-host=http://localhost:<port>/ is added to the node.

This was done so that the LSP methods have an easy way to look up the tree in the DOM to know what end point to issue the LSP commands to.

  1. apiRequire() was renamed to dotnet_require and added to the root object so it's available wherever necessary.
  2. Used MutationObserver to detect when new cells are added to a notebook; this is easier than using the built-in CodeMirror.defineInitHook() function, because that function is called before the editor is attached to the DOM, so we have no way of knowing if the new editor is part of our notebook or another one. The MutationObserver method solves this.
@brettfo brettfo force-pushed the httlsp branch 3 times, most recently from 478ad76 to 12a3fba Mar 13, 2020
@brettfo brettfo force-pushed the httlsp branch 2 times, most recently from bc3b3cf to 43ab009 Mar 16, 2020
@brettfo brettfo merged commit 34717a5 into dotnet:master Mar 16, 2020
6 checks passed
@brettfo brettfo deleted the httlsp branch Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment