Skip to content

Conversation

@madhur310
Copy link
Collaborator

@W-20064193@
move lls and remove fs references

Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

meta: lets get the dotfiles (tsconfig, eslint, etc) and build dirs harmonized with the rest of the monorepo.

I think there's some unnecessary exports and once you clean those up it'll reveal more dead code

I didn't expect jorje jar to change

* Manages file system data received from the LSP client
* This replaces direct file system access in the language server
*/
export class FileSystemDataProvider implements IFileSystemProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you haven't already, I'd like you to get Pete's verdict on this. It was my understanding the that the LS would need to run as its own process via a worker and what's passed in would be primitives.

Perhaps I misunderstood. Or maybe these all run "inside" the extension rather than as a separate process and it's just "different" from apex that way?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mshanemc The LSP protocol uses JSON for request payloads, so anything that can be properly converted to JSON can be used.

The client side of LLS is using function serialize to ensure only the data is included in the payload.

Copy link
Contributor

Choose a reason for hiding this comment

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

The custom endpoint that Kyle created to pass the std apex class zip from client to server gives us evidence shows that streaming is used on the client/server connection. Payload size may not be an issue for the connection, but transferring contents of workspace during init may impact perceived time to ready.

The approach in the new LS is to use std LSP didOpen requests for each file found in the workspace. Madhur and I have discussed that option as well.

As you know, that approach may not be the correct one given the perf issues we have been discussing.

Copy link
Collaborator Author

@madhur310 madhur310 Nov 6, 2025

Choose a reason for hiding this comment

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

@mshanemc yes you are right - this is a temp approach I implemented to get everything else working. Currently I am sending the entire workspace as initParam which won't work for bigger workspaces and have perceivable start up delays.

but transferring contents of workspace during init may impact perceived time to ready

@peternhale agreed on that, the current approach in this PR is temperory & is following:

  • Client populates & serializes entires workspaces -> add to init options -> server deserializes and uses it.

I am working through you workspaceLoader poc, that would make it:

  • Client initializes server (without any fileSystemProvider) -> Client fires a bunch of onTextDocumentOpen -> Server uses it to populate the fileSystemDataProvider object it needs

Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

left some questions, some are still open. Can you mark resolved anything that we don't need anymore?

if (jsconfigExists) {
const existingConfigContent = this.fileSystemProvider.getFileContent(jsconfigPath);
if (!existingConfigContent) {
throw new Error('Existing config not found');
Copy link
Contributor

Choose a reason for hiding this comment

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

why? line 356 checked that it was there?

Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

Got through the rest of it. There's a lot of code that shouldn't exist

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.

4 participants