Prepare VS Code extension for marketplace preview #884
Conversation
…tically restarting when changing useTsgo
| const packagePath = workspaceResolve(exe); | ||
| const packageJsonPath = vscode.Uri.joinPath(packagePath, "package.json"); | ||
| const packageJson = JSON.parse(await vscode.workspace.fs.readFile(packageJsonPath).then(buffer => buffer.toString())); | ||
| const getExePath = (await import(vscode.Uri.joinPath(packagePath, "lib", "getExePath.js").toString())).default; |
There was a problem hiding this comment.
Does this require modifying the package to be importable? IIRC this isn't exported on the package. Does this only work because the emitted code is CJS?
We should probably just formally export it (and maybe drop the default export?).
There was a problem hiding this comment.
No, this works because it’s an absolute path, so the package.json is irrelevant.
There was a problem hiding this comment.
Huh, I didn't realize that absolute resolution bypassed export rules.
There was a problem hiding this comment.
Also relative. Only bare specifiers do the complicated thing. The idea being that ESM resolution should generally be predictable and cheap, but package specifier resolution is what it is.
| } | ||
|
|
||
| export function getBuiltinExePath(context: vscode.ExtensionContext): string { | ||
| return context.asAbsolutePath(path.join("./lib", `tsgo${process.platform === "win32" ? ".exe" : ""}`)); |
There was a problem hiding this comment.
I'd like to be able to move this to node_modules so we can just use resolve, but vsce is making it very hard to do that due to microsoft/vscode-vsce#970, so I think we'll have to keep this for now. Thankfully it's in the VSIX so is changeable, unlike the other resolution algorithm.
| const output = vscode.window.createOutputChannel("typescript-native-preview", "log"); | ||
| const traceOutput = vscode.window.createOutputChannel("typescript-native-preview (LSP)"); | ||
| const client = new Client(output, traceOutput); | ||
| registerCommands(context, client, output, traceOutput); |
There was a problem hiding this comment.
Can we defer output window creation until Client actually starts the language server?
For the "show output" command, I think we could just make the command ask the Client for its output window (or undefined).
Probably not required for this PR, though, just noticing it now.
There was a problem hiding this comment.
Maybe we can just defer the trace output; I have a message in the main output window saying that the extension is disabled whenever typescript.experimental.useTsgo isn’t set.
There was a problem hiding this comment.
I don't really mind; this seems fine for now.
There was a problem hiding this comment.
LGTM; will defer to @DanielRosenwasser about the package.json naming
and stuff.
| @@ -1208,6 +1208,6 @@ export const installExtension = task({ | |||
| console.log(pc.yellowBright("\nExtension installed. ") + "To enable this extension, set:\n"); | |||
| console.log(pc.whiteBright(` "typescript.experimental.useTsgo": true\n`)); | |||
There was a problem hiding this comment.
Is this going to be the final setting name?
There was a problem hiding this comment.
It's hardcoded in https://github.com/microsoft/vscode/blob/35d9d31a8c96ba3e545a1db67a91dc9876551cc9/extensions/typescript-language-features/src/extension.ts#L37 so we can't really change it immediately
There was a problem hiding this comment.
@mjbvz can we at least change how this looks on the UI so Tsgo is not awkwardly Pascal-cased?
There was a problem hiding this comment.
Mentioned here that extensions may not be able to change it: microsoft/vscode#73374
@rzhao271 Can let the TS team know if there's a way around the auto casing in the settings UI
There was a problem hiding this comment.
I see the Tsgo setting in the package.json file.
There's no workaround yet. What is the expected display name, "Use TS Go"?
There was a problem hiding this comment.
Worth noting users don’t need to see this setting if they use this
andrewbranch
merged commit af312b3
into
microsoft:jabaile/native-preview
This builds on top of #876 to update strings in the extension, allow resolution of the exe from the @typescript/native-preview package, and make minor UX improvements.