Skip to content

Build LSP directly in destination so source maps are correct #749

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

Merged
merged 3 commits into from
Jul 4, 2025

Conversation

lionel-
Copy link
Collaborator

@lionel- lionel- commented Jul 3, 2025

We were building inside apps/lsp/dist, then copying over to apps/vscode/out/lsp. But the source maps are relative so they were no longer pointing to the correct locations.

Since this is a mono repo, I thought we could just directly build in the vscode's output directory.

With this change, we can set breakpoints inside the LSP. You'll first need to use the "Attach to LSP" launch action once the LSP process has been spawned.

@cscheid
Copy link
Contributor

cscheid commented Jul 3, 2025

@juliasilge is this change safe for you? I wonder specifically if the action that builds the vsix installers will work under this change.

@lionel-
Copy link
Collaborator Author

lionel- commented Jul 3, 2025

Good point, I've checked that vsce package still works locally but I should do a manual run of the action on this PR.

@lionel-
Copy link
Collaborator Author

lionel- commented Jul 3, 2025

hmm that action was running automatically in #748 but here it didn't start.

@lionel-
Copy link
Collaborator Author

lionel- commented Jul 3, 2025

okay the GH actions were not watching over apps/lsp changes, now fixed

@cscheid
Copy link
Contributor

cscheid commented Jul 3, 2025

Do similar changes need to happen in https://github.com/quarto-dev/quarto/blob/main/.github/workflows/publish.yaml? There's references to apps/vscode there as well.

@lionel-
Copy link
Collaborator Author

lionel- commented Jul 3, 2025

I don't think so. We don't need to watch LSP files in that workflow and regarding build dependencies, the dependency of apps/vscode on apps/lsp is internally defined. There is a prepublish step that calls turbo run build at top-level before building the extension:

"vscode:prepublish": "rm -rf ./out/markdownit && cd ../.. && turbo run build --force --filter quarto...",

This ensures apps/lsp is built (into vscode's out directory with the changes in this PR). I was curious how that worked and found out that's because all subfolders in apps/ are defined as Turbo workspaces there:

"apps/*",
. turbo looks for dependencies, including internal ones, in each package.json file. Since quarto-lsp (the module name of apps/lsp/) is a dendency of apps/vscode, the build order is guaranteed.

I wonder if we're actually building apps/vscode twice, once in the prepublish step and once in the build step. 🤔

Copy link
Collaborator

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

It took me a couple of tries to get it to work, but I did manage to set a breakpoint in the LSP with this PR.

How would you feel about adding something in README.md or a new CONTRIBUTING.md about how to get that set up?

I cannot tell if we are building apps/vscode twice. 😬

@lionel- lionel- force-pushed the bugfix/lsp-source-maps branch from 3624d2f to e07fe28 Compare July 4, 2025 06:45
@lionel- lionel- merged commit 8f99af7 into main Jul 4, 2025
1 check passed
@lionel- lionel- deleted the bugfix/lsp-source-maps branch July 4, 2025 06:46
lionel- added a commit that referenced this pull request Jul 7, 2025
* Build LSP directly in destination so source maps are correct

* Fix LSP `outFiles`

* Trigger workflows on LSP changes
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.

3 participants