-
Notifications
You must be signed in to change notification settings - Fork 12
Migrate to Volar #388
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
base: main
Are you sure you want to change the base?
Migrate to Volar #388
Conversation
|
@@ -161,12 +163,12 @@ export class Extracted { | |||
* on the view type. | |||
*/ | |||
abstract class TokenView { | |||
#tokens: Token[]; | |||
tokens: Token[]; |
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.
This is an awful hack so that I can generate Volar CodeMapping
objects from Tokens.
A better solution would be to have the extractor be able to generate CodeMapping
s itself...
"command": "marko.debug.showScriptOutput", | ||
"title": "Show Extracted Script Output", | ||
"category": "Marko (Debug)" |
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.
// TODO: Is this necessary? | ||
// provideDocumentSymbols(document, token) { | ||
// if (token.isCancellationRequested) return; | ||
// return worker(document, (virtualCode) => { | ||
// return provideDocumentSymbols(virtualCode); | ||
// }); | ||
// }, |
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.
I'm probably missing something - but I wasn't sure if Marko specific document symbols were still necessary and that couldn't be provided by TS or HTML.
import type { Diagnostic } from "@volar/language-server"; | ||
import { TextDocument } from "vscode-languageserver-textdocument"; | ||
|
||
export function enhanceDiagnosticPositions( |
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.
This is necessary to ensure we show diagnostics. Volar doesn't seem to display them when they are larger than the code mappings, so I clamp the diagnostics to the most reasonable code mapping we can find...
There is almost certainly a better way, but this seems good enough for now?
// This will cause the plugin to be called again, so we check that the extension is not already added. | ||
ps.setHostConfiguration({ | ||
extraFileExtensions: extraExtensions.concat( | ||
Processors.extensions.map((extension) => ({ |
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.
I notice we're using Processors here, but extractors elsewhere... I'm wondering if we should be using Processors in the MarkoVirtualCode?
It seems there is a problem here that in the LSP value shorthands don't report errors for some reason:
Maybe a problem with zero sized spans or something? |
e2bd9fe
to
4d516ad
Compare
Yeah - good catch. There is a zero-sized span for the value shorthand. ![]() Using Volar Labs. |
Yes indeed, I worked around it like this: diff --git a/packages/language-server/src/index.ts b/packages/language-server/src/index.ts
index 44fa55a..5fb32ad 100644
--- a/packages/language-server/src/index.ts
+++ b/packages/language-server/src/index.ts
@@ -5,6 +5,7 @@ import {
loadTsdkByPath,
} from "@volar/language-server/node";
import { URI } from "vscode-uri";
+import { defaultMapperFactory } from "@volar/language-core";
import { addMarkoTypes, createMarkoLanguagePlugin } from "./language";
import { getLanguageServicePlugins } from "./plugins";
@@ -37,13 +38,27 @@ connection.onInitialize((params) => {
uri.fsPath.replace(/\\/g, "/"),
),
],
- setup({ project }) {
+ setup({ project, language }) {
const { languageServiceHost, configFileName } = project.typescript!;
const rootPath = configFileName
? configFileName.split("/").slice(0, -1).join("/")
: env.workspaceFolders[0]!.fsPath;
+ language.mapperFactory = (mappings) => {
+ let def = defaultMapperFactory(mappings);
+
+ for (const map of def.mappings) {
+ for (let i = 0; i < map.lengths.length; i++) {
+ if (map.lengths[i] === 0) {
+ map.lengths[i] = 1;
+ }
+ }
+ }
+
+ return def;
+ };
+
addMarkoTypes(rootPath, typescript, languageServiceHost);
},
}; Seems to work for me, but I'm not sure if that is an alright solution or not, I know nothing about volar. Also this mapperFactory api seems to be related to that "Improve diagnostic mapping" point? |
With regards to "Could we use Volar for marko-type-check?", I actually managed to do that it seems: This is related to the issue volarjs/volar.js#145 Right now it is messy I will try to put a PR into your branch after a cleanup. |
Another thing I found is that there are problems with type inference on dynamic tag imports, even though they work okay for static imports.
|
This is a clean version of #294.
This is a proof of concept that shows what it might look like if the Marko Language Tools migrated to Volar as a base.
This proof-of-concept is roughly complete for our purposes, we've been using it for the past few months.
Motivation and Context
Migrating to Volar resolves the issues we've had with the VSCode plugin on Windows (#240), and it improves the responsiveness of the plugin on Windows.
Why Volar?
MarkoFile
->MarkoVirtualCode
Plugin
->LanguageServicePlugin
TODO
This is fairly untested, I mostly hacked it up so it works with our project.
Questions