-
-
Notifications
You must be signed in to change notification settings - Fork 203
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
base: master
Are you sure you want to change the base?
Conversation
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.
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?
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. |
Co-authored-by: Simon H <[email protected]>
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 👍 |
Okay, so after a bit more testing, I've realised that dealing with the untitled document. The Untitled document is handled with a URI scheme of I couldn't find any official list in VSCode, but the URI schemes that could potentially be a problem are:
Unfortunately, the Best as I can tell, there are three options for properly supporting non-vdoc URIs:
Straightforward, but does limit only one extension to register a scheme provider at a time, so not ideal.
I'm happy to start exploring the last option and see how big of a change this would be. Let me know your thoughts on this. |
Sure, go ahead exploring 👍
|
I think the problem isn't that TypeScript only handles physical files. The problem with completion is the inconsistency of the document URI schema. What is sent back in |
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
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 inlanguage-server/utils.ts
will assume thefile://
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
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!