From 9e583fa562322bfba95ec06c0537d112f51d61eb Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 10 Dec 2021 12:01:35 -0600 Subject: [PATCH] Add separate function for VS Code arguments (#4599) The problem before was that the pop() caused the open in existing instance functionality to break because the arguments no longer contained the file. We could simply remove the pop() but since `workspace` and `folder` are not CLI arguments I think it makes sense to handle them in a separate function which can be called at the point where they are needed. This also lets us de-duplicate some logic since we create these arguments in two spots and lets us skip this logic when we do not need it. The pop() is still avoided because manipulating a passed-in object in-place seems like a risky move. If we really need to do this we should copy the positional argument array instead. --- src/node/cli.ts | 55 +++++++++++++-------- src/node/main.ts | 11 +---- src/node/routes/vscode.ts | 11 +---- test/unit/node/cli.test.ts | 91 ++++++++++++++++++++++++++++------- test/unit/node/plugin.test.ts | 2 - 5 files changed, 112 insertions(+), 58 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index 31ad9a1c..7806c420 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -405,7 +405,10 @@ export const parse = ( throw new Error("--cert-key is missing") } - logger.debug(() => ["parsed command line", field("args", { ...args, password: undefined })]) + logger.debug(() => [ + `parsed ${opts?.configFile ? "config" : "command line"}`, + field("args", { ...args, password: undefined }), + ]) return args } @@ -430,8 +433,6 @@ export interface DefaultedArgs extends ConfigArgs { "user-data-dir": string /* Positional arguments. */ _: [] - folder: string - workspace: string } /** @@ -539,25 +540,8 @@ export async function setDefaults(cliArgs: UserProvidedArgs, configArgs?: Config args._ = [] } - let workspace = "" - let folder = "" - if (args._.length) { - const lastEntry = path.resolve(process.cwd(), args._[args._.length - 1]) - const entryIsFile = await isFile(lastEntry) - - if (entryIsFile && path.extname(lastEntry) === ".code-workspace") { - workspace = lastEntry - args._.pop() - } else if (!entryIsFile) { - folder = lastEntry - args._.pop() - } - } - return { ...args, - workspace, - folder, usingEnvPassword, usingEnvHashedPassword, } as DefaultedArgs // TODO: Technically no guarantee this is fulfilled. @@ -760,3 +744,34 @@ export const shouldOpenInExistingInstance = async (args: UserProvidedArgs): Prom return undefined } + +/** + * Convert our arguments to VS Code server arguments. + */ +export const toVsCodeArgs = async (args: DefaultedArgs): Promise => { + let workspace = "" + let folder = "" + if (args._.length) { + const lastEntry = path.resolve(args._[args._.length - 1]) + const entryIsFile = await isFile(lastEntry) + if (entryIsFile && path.extname(lastEntry) === ".code-workspace") { + workspace = lastEntry + } else if (!entryIsFile) { + folder = lastEntry + } + // Otherwise it is a regular file. Spawning VS Code with a file is not yet + // supported but it can be done separately after code-server spawns. + } + + return { + "connection-token": "0000", + ...args, + workspace, + folder, + "accept-server-license-terms": true, + /** Type casting. */ + help: !!args.help, + version: !!args.version, + port: args.port?.toString(), + } +} diff --git a/src/node/main.ts b/src/node/main.ts index 1bda3b74..eb5a5be0 100644 --- a/src/node/main.ts +++ b/src/node/main.ts @@ -5,7 +5,7 @@ import path from "path" import { Disposable } from "../common/emitter" import { plural } from "../common/util" import { createApp, ensureAddress } from "./app" -import { AuthType, DefaultedArgs, Feature, UserProvidedArgs } from "./cli" +import { AuthType, DefaultedArgs, Feature, toVsCodeArgs, UserProvidedArgs } from "./cli" import { coderCloudBind } from "./coder_cloud" import { commit, version } from "./constants" import { register } from "./routes" @@ -35,14 +35,7 @@ export const runVsCodeCli = async (args: DefaultedArgs): Promise => { const spawnCli = await loadAMDModule("vs/server/remoteExtensionHostAgent", "spawnCli") try { - await spawnCli({ - ...args, - /** Type casting. */ - "accept-server-license-terms": true, - help: !!args.help, - version: !!args.version, - port: args.port?.toString(), - }) + await spawnCli(await toVsCodeArgs(args)) } catch (error: any) { logger.error("Got error from VS Code", error) } diff --git a/src/node/routes/vscode.ts b/src/node/routes/vscode.ts index c78543e6..4d394c25 100644 --- a/src/node/routes/vscode.ts +++ b/src/node/routes/vscode.ts @@ -3,6 +3,7 @@ import * as express from "express" import { WebsocketRequest } from "../../../typings/pluginapi" import { logError } from "../../common/util" import { isDevMode } from "../constants" +import { toVsCodeArgs } from "../cli" import { ensureAuthenticated, authenticated, redirect } from "../http" import { loadAMDModule, readCompilationStats } from "../util" import { Router as WsRouter } from "../wsRouter" @@ -87,15 +88,7 @@ export class CodeServerRouteWrapper { ) try { - this._codeServerMain = await createVSServer(null, { - "connection-token": "0000", - "accept-server-license-terms": true, - ...args, - /** Type casting. */ - help: !!args.help, - version: !!args.version, - port: args.port?.toString(), - }) + this._codeServerMain = await createVSServer(null, await toVsCodeArgs(args)) } catch (createServerError) { logError(logger, "CodeServerRouteWrapper", createServerError) return next(createServerError) diff --git a/test/unit/node/cli.test.ts b/test/unit/node/cli.test.ts index 0dcf24f4..e49794d5 100644 --- a/test/unit/node/cli.test.ts +++ b/test/unit/node/cli.test.ts @@ -12,10 +12,26 @@ import { setDefaults, shouldOpenInExistingInstance, splitOnFirstEquals, + toVsCodeArgs, } from "../../../src/node/cli" import { shouldSpawnCliProcess } from "../../../src/node/main" import { generatePassword, paths } from "../../../src/node/util" -import { useEnv, tmpdir } from "../../utils/helpers" +import { clean, useEnv, tmpdir } from "../../utils/helpers" + +// The parser should not set any defaults so the caller can determine what +// values the user actually set. These are only set after explicitly calling +// `setDefaults`. +const defaults = { + auth: "password", + host: "localhost", + port: 8080, + "proxy-domain": [], + usingEnvPassword: false, + usingEnvHashedPassword: false, + "extensions-dir": path.join(paths.data, "extensions"), + "user-data-dir": paths.data, + _: [], +} describe("parser", () => { beforeEach(() => { @@ -24,23 +40,6 @@ describe("parser", () => { console.log = jest.fn() }) - // The parser should not set any defaults so the caller can determine what - // values the user actually set. These are only set after explicitly calling - // `setDefaults`. - const defaults = { - auth: "password", - host: "localhost", - port: 8080, - "proxy-domain": [], - usingEnvPassword: false, - usingEnvHashedPassword: false, - "extensions-dir": path.join(paths.data, "extensions"), - "user-data-dir": paths.data, - _: [], - workspace: "", - folder: "", - } - it("should parse nothing", async () => { expect(parse([])).toStrictEqual({}) }) @@ -667,3 +666,59 @@ describe("readSocketPath", () => { expect(contents2).toBe(contents1) }) }) + +describe("toVsCodeArgs", () => { + const vscodeDefaults = { + ...defaults, + "connection-token": "0000", + "accept-server-license-terms": true, + help: false, + port: "8080", + version: false, + } + + beforeAll(async () => { + // Clean up temporary directories from the previous run. + await clean("vscode-args") + }) + + it("should convert empty args", async () => { + expect(await toVsCodeArgs(await setDefaults(parse([])))).toStrictEqual({ + ...vscodeDefaults, + folder: "", + workspace: "", + }) + }) + + it("should convert with workspace", async () => { + const workspace = path.join(await tmpdir("vscode-args"), "test.code-workspace") + await fs.writeFile(workspace, "foobar") + expect(await toVsCodeArgs(await setDefaults(parse([workspace])))).toStrictEqual({ + ...vscodeDefaults, + workspace, + folder: "", + _: [workspace], + }) + }) + + it("should convert with folder", async () => { + const folder = await tmpdir("vscode-args") + expect(await toVsCodeArgs(await setDefaults(parse([folder])))).toStrictEqual({ + ...vscodeDefaults, + folder, + workspace: "", + _: [folder], + }) + }) + + it("should ignore regular file", async () => { + const file = path.join(await tmpdir("vscode-args"), "file") + await fs.writeFile(file, "foobar") + expect(await toVsCodeArgs(await setDefaults(parse([file])))).toStrictEqual({ + ...vscodeDefaults, + folder: "", + workspace: "", + _: [file], + }) + }) +}) diff --git a/test/unit/node/plugin.test.ts b/test/unit/node/plugin.test.ts index 5e23c24b..acd41731 100644 --- a/test/unit/node/plugin.test.ts +++ b/test/unit/node/plugin.test.ts @@ -42,8 +42,6 @@ describe("plugin", () => { usingEnvHashedPassword: false, "extensions-dir": "", "user-data-dir": "", - workspace: "", - folder: "", } next() }