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

feat: Allow embedded languages to utilise Svelte Intellisense in VSCode #2611

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BOJIT
Copy link

@BOJIT BOJIT commented Nov 30, 2024

Hello! This PR modifies the behaviour of the Svelte VSCode extension so that it activates for URIs that don't use the file:// scheme.
During standard extension usage, this makes no difference, as all Svelte files on disk will use the file:// scheme in their URI. However, enabling this support allows other extensions to use Svelte as an embedded language and still take full advantage of all of the Svelte extension's Intellisense.

This is handled in VSCode by a mechanism called Request Forwarding. In summary, VSCode will preprocess the custom language and create a virtual document that contains just the "Svelte" portion of the document. It can then invoke the Svelte extension on that virtual document.
The problem is that these virtual documents must use a unique URI scheme. As Svelte currently only activates on file:// scheme URIs, these requests will be dropped.

Example Use-Cases

  • MDsveX syntax support: I've got a draft extension that adds MDsveX syntax support to VSCode.
  • Custom languages built around Svelte. I know that Evidence BI maintain an extension that would benefit from Svelte intellisense/language features.

Potential Problems

Various parts of the extension (document snapshots, Typescript plugin) track the path of the file, not the URI. The conversion from path to URI in language-server/utils.ts will assume the file:// URI scheme when mapping between paths and URIs.

I am not familiar enough with the Svelte Language Server inner workings to fully know what the impact of supporting all URI schemes is. I've tested these modifications locally and the extension seems to behave identically to the original extension, but I would appreciate some advice from the maintainers on the impact of this change!

I'm also not sure if there are any URI schemes that should be explicitly disallowed by the extension. For example, http/https URIs ending in .svelte should probably not be recorded in document snapshots, as these might appear in webviews?

Next Steps

  • My suggestion would be to only ever pass references to URIs in the language server, and only convert them into paths where speific APIs require it.
  • Some of the Svelte-Kit specific Intellisense still doesn't work in virtual documents. Not sure where this gets tracked in the extension.

I'm hoping that this small change does not break any existing behaviour, as merging it will make any downstream embedded svelte extension's experience much better!

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed PR description! I'm ok to give this a try under the assumption that we're not going to jump through countless hoops to make it work 100% correctly for all use cases.

@jasonlyu123 any objections? Anything you think that might need to be adjusted / could go wrong?

packages/language-server/src/utils.ts Outdated Show resolved Hide resolved
@jasonlyu123
Copy link
Member

We need some extra work with the unsaved "untitled" document. It is enabled in this PR but it seems like it sometimes causes crashes during completion. Might need to check if other stuff throws errors too.

@BOJIT
Copy link
Author

BOJIT commented Dec 4, 2024

Ah, good point! Didn't test untitled documents... Will have another look this week and check a few additional cases.

I imagine that I'll may need to raise some future PRs to fully support every embedded language forwarding feature, but the priority in this PR is to ensure that it doesn't break any existing Svelte extension features!

I'll do some further testing 👍

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