-
Notifications
You must be signed in to change notification settings - Fork 33k
Goto definition for built-in symbols in HTML script #244074
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
Conversation
@microsoft-github-policy-service agree |
@@ -601,3 +611,11 @@ function generateIndent(level: number, options: FormattingOptions) { | |||
return repeat('\t', level); | |||
} | |||
} | |||
|
|||
function getLibDomUriAndDocument(): [string, TextDocument] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To compute the library location, add a new function to
const serverFolder = basename(__dirname) === 'dist' ? dirname(__dirname) : dirname(dirname(__dirname)); |
As for the document URI, this is trickier as this also needs to run on the web (vscode.dev), and there's no 'file' there.
You probably need to start from the extensionUri that you get on the HTML client, and pass it on the server.
if (!content) { | ||
return undefined; | ||
} | ||
const libDocument = TextDocument.create(libUri, 'typescript', 1, content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aeschli Currently I put the logic here instead of in javascriptLibs.ts
because webpack (for vscode.dev) uses a different version for it, as specified in html-language-features/server/extension-browser.webpack.config.js
. Should I declare this logic in javascriptLibs.ts
and edit the file html-language-features/server/build/javaScriptLibraryLoader.js
as well, or should I just keep the logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, now I remember, for vscode.dev we just bundle the library files.
Sorry, it's been a while since I did LSP stuff, but I start to remember again. This allows the server to provide documents with virtual URIs that then can be opened by the client (VS Code).
What do you think? I think that's the right approach, which will also work on vscode.dev. |
I think that's a good approach, but I want to clarify a few things before I work on the new approach.
Are there other things I am missing that makes it not compatible on vscode.dev?
|
Ah nice it works. What I don't like is that it's not reflecting what we really do (using a bundled file, as we found out). So we could change that, and go away from bunding, but that's another challenge as reading URIs from the server is also not possible (needs to go to the client and the vscode filesystem API) I think you need to set this to the server capabilities:
|
@aeschli I have implemented the new endpoint in the server to serve file content. But one downside to it is that upon opening the definition file, the file header does not show. In contrast to The behaviour is the same both on web and on desktop. For desktop, upon reloading the window, the file is no longer displayed. I'm not sure if this behaviour can be easily fixed, as the |
You mean the breadcrumbs. Don't know why they don't show, you have to debug in the breadcrumb code. |
@aeschli Here's what I found: For breadcrumb to be displayed with files from vscode.workspace.registerFileSystemProvider('html-server', /* some file system provider */); However, for a client-server extension like For now, I believe the solution is to declare the file system provider as above in the client, and then within the provider, make a call to the server to fetch the file content via What do you think? Lmk how I should proceed. |
Thanks for the analysis! I see, the LSP client uses the 'TextDocumentContentProvider' to serve the document, not a file system provider. https://github.com/microsoft/vscode-languageserver-node/blob/df05883f34b39255d40d68cef55caf2e93cff35f/client/src/common/textDocumentContent.ts#L86 I guess we could file an issue against the LSP client to use a file system provider instead. @dbaeumer FYI. That would help other languages as well that use TextDocumentContents (I believe Java) I don't know what happens if there's both a vscode.TextDocumentContentProvider as well as a FileSystemProvider. |
@aeschli As for persisting the document through restart, I found out that there must be an editor serializer ( I have tried looking through VSCode API, the closest ones that I can find are As for now, the only solution I can think of is to add a serializer for |
@aeschli Other than these 2 issues, the PR is ready to be reviewed. |
@nknguyenhc Very nice and slick! Thanks a lot! I allowed myself to do a change on the signature of |
Yep, looks great! |
Fixes #236603
I handle the filename
lib.dom.d.ts
and resolves it to the correct file to view definition of built-in DOM classes.