From 78658f1cf48a5e019a82cde937cfa8feed8b986b Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Wed, 2 Mar 2022 15:36:38 -0700 Subject: [PATCH] refactor: remove folder/workspace from vsCodeCliArgs (#4932) * refactor: remove folder/workspace from vsCodeCliArgs Since we handle this in the vscode.ts route, we no longer need to pass it to VS Code as a CLI arg since it's deprecated on that side. * feat(vscode): redirect to folder from cli * Update src/node/routes/vscode.ts Co-authored-by: Asher * fixup!: update _: type * fixup!: move vars to lower if block * fixup!: share redirect block * fixup!: mmove req.query.ew block into if * fixup!: refactor vscode tests * refactor: make vscode.ts logic easier to read * fixup!: fix broken tests and clean up logic * chore: upgrade vscode version * fixup!: delete unnecessary if closed block * Update src/node/routes/vscode.ts Co-authored-by: Asher * fixup!: rename to FOLDER_OR_WORKSPACE_WAS_CLOSED Co-authored-by: Asher --- src/node/cli.ts | 28 +-------- src/node/routes/vscode.ts | 51 ++++++++++------ test/unit/node/cli.test.ts | 25 -------- test/unit/node/routes/vscode.test.ts | 91 +++++++++++----------------- vendor/package.json | 2 +- vendor/yarn.lock | 4 +- 6 files changed, 73 insertions(+), 128 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index e996f3412..9928db1c6 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -3,15 +3,7 @@ import { promises as fs } from "fs" import yaml from "js-yaml" import * as os from "os" import * as path from "path" -import { - canConnect, - generateCertificate, - generatePassword, - humanPath, - paths, - isNodeJSErrnoException, - isFile, -} from "./util" +import { canConnect, generateCertificate, generatePassword, humanPath, paths, isNodeJSErrnoException } from "./util" const DEFAULT_SOCKET_PATH = path.join(os.tmpdir(), "vscode-ipc") @@ -448,7 +440,7 @@ export interface DefaultedArgs extends ConfigArgs { "extensions-dir": string "user-data-dir": string /* Positional arguments. */ - _: [] + _: string[] } /** @@ -770,25 +762,9 @@ export const shouldOpenInExistingInstance = async (args: UserProvidedArgs): Prom * 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, diff --git a/src/node/routes/vscode.ts b/src/node/routes/vscode.ts index fe9c44181..b137bfcd5 100644 --- a/src/node/routes/vscode.ts +++ b/src/node/routes/vscode.ts @@ -1,12 +1,13 @@ import { logger } from "@coder/logger" import * as express from "express" +import * as path from "path" import { WebsocketRequest } from "../../../typings/pluginapi" import { logError } from "../../common/util" import { toVsCodeArgs } from "../cli" import { isDevMode } from "../constants" import { authenticated, ensureAuthenticated, redirect, self } from "../http" import { SocketProxyProvider } from "../socket" -import { loadAMDModule } from "../util" +import { isFile, loadAMDModule } from "../util" import { Router as WsRouter } from "../wsRouter" import { errorHandler } from "./errors" @@ -25,6 +26,9 @@ export class CodeServerRouteWrapper { private $root: express.Handler = async (req, res, next) => { const isAuthenticated = await authenticated(req) + const NO_FOLDER_OR_WORKSPACE_QUERY = !req.query.folder && !req.query.workspace + // Ew means the workspace was closed so clear the last folder/workspace. + const FOLDER_OR_WORKSPACE_WAS_CLOSED = req.query.ew if (!isAuthenticated) { const to = self(req) @@ -33,25 +37,38 @@ export class CodeServerRouteWrapper { }) } - const { query } = await req.settings.read() - if (query) { - // Ew means the workspace was closed so clear the last folder/workspace. - if (req.query.ew) { - delete query.folder - delete query.workspace - } + if (NO_FOLDER_OR_WORKSPACE_QUERY && !FOLDER_OR_WORKSPACE_WAS_CLOSED) { + const settings = await req.settings.read() + const lastOpened = settings.query || {} + // This flag disables the last opened behavior + const IGNORE_LAST_OPENED = req.args["ignore-last-opened"] + const HAS_LAST_OPENED_FOLDER_OR_WORKSPACE = lastOpened.folder || lastOpened.workspace + const HAS_FOLDER_OR_WORKSPACE_FROM_CLI = req.args._.length > 0 + const to = self(req) + + let folder = undefined + let workspace = undefined // Redirect to the last folder/workspace if nothing else is opened. - if ( - !req.query.folder && - !req.query.workspace && - (query.folder || query.workspace) && - !req.args["ignore-last-opened"] // This flag disables this behavior. - ) { - const to = self(req) + if (HAS_LAST_OPENED_FOLDER_OR_WORKSPACE && !IGNORE_LAST_OPENED) { + folder = lastOpened.folder + workspace = lastOpened.workspace + } else if (HAS_FOLDER_OR_WORKSPACE_FROM_CLI) { + const lastEntry = path.resolve(req.args._[req.args._.length - 1]) + const entryIsFile = await isFile(lastEntry) + const IS_WORKSPACE_FILE = entryIsFile && path.extname(lastEntry) === ".code-workspace" + + if (IS_WORKSPACE_FILE) { + workspace = lastEntry + } else if (!entryIsFile) { + folder = lastEntry + } + } + + if (folder || workspace) { return redirect(req, res, to, { - folder: query.folder, - workspace: query.workspace, + folder, + workspace, }) } } diff --git a/test/unit/node/cli.test.ts b/test/unit/node/cli.test.ts index 327219ea4..108f2e841 100644 --- a/test/unit/node/cli.test.ts +++ b/test/unit/node/cli.test.ts @@ -726,29 +726,6 @@ describe("toVsCodeArgs", () => { 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(testName), "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(testName) - expect(await toVsCodeArgs(await setDefaults(parse([folder])))).toStrictEqual({ - ...vscodeDefaults, - folder, - workspace: "", - _: [folder], }) }) @@ -757,8 +734,6 @@ describe("toVsCodeArgs", () => { await fs.writeFile(file, "foobar") expect(await toVsCodeArgs(await setDefaults(parse([file])))).toStrictEqual({ ...vscodeDefaults, - folder: "", - workspace: "", _: [file], }) }) diff --git a/test/unit/node/routes/vscode.test.ts b/test/unit/node/routes/vscode.test.ts index d896f8467..a4c0ba13a 100644 --- a/test/unit/node/routes/vscode.test.ts +++ b/test/unit/node/routes/vscode.test.ts @@ -1,19 +1,9 @@ import { promises as fs } from "fs" -import { Response } from "node-fetch" import * as path from "path" import { clean, tmpdir } from "../../../utils/helpers" import * as httpserver from "../../../utils/httpserver" import * as integration from "../../../utils/integration" -interface WorkbenchConfig { - folderUri?: { - path: string - } - workspaceUri?: { - path: string - } -} - describe("vscode", () => { let codeServer: httpserver.HttpServer | undefined @@ -39,7 +29,7 @@ describe("vscode", () => { expect(resp.status).toBe(200) const html = await resp.text() const url = new URL(resp.url) // Check there were no redirections. - expect(url.pathname + decodeURIComponent(url.search)).toBe(route) + expect(url.pathname + url.search).toBe(route) switch (route) { case "/": case "/vscode/": @@ -52,52 +42,25 @@ describe("vscode", () => { } }) - /** - * Get the workbench config from the provided response. - */ - const getConfig = async (resp: Response): Promise => { - expect(resp.status).toBe(200) - const html = await resp.text() - const match = html.match(//) - if (!match || !match[1]) { - throw new Error("Unable to find workbench configuration") - } - const config = match[1].replace(/"/g, '"') - try { - return JSON.parse(config) - } catch (error) { - console.error("Failed to parse workbench configuration", config) - throw error - } - } + it("should redirect to the passed in workspace using human-readable query", async () => { + const workspace = path.join(await tmpdir(testName), "test.code-workspace") + await fs.writeFile(workspace, "") + codeServer = await integration.setup(["--auth=none", workspace], "") - it("should have no default folder or workspace", async () => { - codeServer = await integration.setup(["--auth=none"], "") - - const config = await getConfig(await codeServer.fetch("/")) - expect(config.folderUri).toBeUndefined() - expect(config.workspaceUri).toBeUndefined() + const resp = await codeServer.fetch("/") + const url = new URL(resp.url) + expect(url.pathname).toBe("/") + expect(url.search).toBe(`?workspace=${workspace}`) }) - it("should have a default folder", async () => { - const defaultDir = await tmpdir(testName) - codeServer = await integration.setup(["--auth=none", defaultDir], "") + it("should redirect to the passed in folder using human-readable query", async () => { + const folder = await tmpdir(testName) + codeServer = await integration.setup(["--auth=none", folder], "") - // At first it will load the directory provided on the command line. - const config = await getConfig(await codeServer.fetch("/")) - expect(config.folderUri?.path).toBe(defaultDir) - expect(config.workspaceUri).toBeUndefined() - }) - - it("should have a default workspace", async () => { - const defaultWorkspace = path.join(await tmpdir(testName), "test.code-workspace") - await fs.writeFile(defaultWorkspace, "") - codeServer = await integration.setup(["--auth=none", defaultWorkspace], "") - - // At first it will load the workspace provided on the command line. - const config = await getConfig(await codeServer.fetch("/")) - expect(config.folderUri).toBeUndefined() - expect(config.workspaceUri?.path).toBe(defaultWorkspace) + const resp = await codeServer.fetch("/") + const url = new URL(resp.url) + expect(url.pathname).toBe("/") + expect(url.search).toBe(`?folder=${folder}`) }) it("should redirect to last query folder/workspace", async () => { @@ -105,6 +68,7 @@ describe("vscode", () => { const folder = await tmpdir(testName) const workspace = path.join(await tmpdir(testName), "test.code-workspace") + await fs.writeFile(workspace, "") let resp = await codeServer.fetch("/", undefined, { folder, workspace, @@ -118,7 +82,7 @@ describe("vscode", () => { resp = await codeServer.fetch(route) const url = new URL(resp.url) expect(url.pathname).toBe(route) - expect(decodeURIComponent(url.search)).toBe(`?folder=${folder}&workspace=${workspace}`) + expect(url.search).toBe(`?folder=${folder}&workspace=${workspace}`) await resp.text() } @@ -126,13 +90,24 @@ describe("vscode", () => { resp = await codeServer.fetch("/", undefined, { ew: "true" }) let url = new URL(resp.url) expect(url.pathname).toBe("/") - expect(decodeURIComponent(url.search)).toBe("?ew=true") + expect(url.search).toBe("?ew=true") await resp.text() resp = await codeServer.fetch("/") url = new URL(resp.url) expect(url.pathname).toBe("/") - expect(decodeURIComponent(url.search)).toBe("") + expect(url.search).toBe("") + await resp.text() + }) + + it("should do nothing when nothing is passed in", async () => { + codeServer = await integration.setup(["--auth=none"], "") + + let resp = await codeServer.fetch("/", undefined) + + expect(resp.status).toBe(200) + const url = new URL(resp.url) + expect(url.search).toBe("") await resp.text() }) @@ -141,6 +116,8 @@ describe("vscode", () => { const folder = await tmpdir(testName) const workspace = path.join(await tmpdir(testName), "test.code-workspace") + await fs.writeFile(workspace, "") + let resp = await codeServer.fetch("/", undefined, { folder, workspace, @@ -152,7 +129,7 @@ describe("vscode", () => { resp = await codeServer.fetch("/") const url = new URL(resp.url) expect(url.pathname).toBe("/") - expect(decodeURIComponent(url.search)).toBe("") + expect(url.search).toBe("") await resp.text() }) }) diff --git a/vendor/package.json b/vendor/package.json index 82feee23e..9d00927be 100644 --- a/vendor/package.json +++ b/vendor/package.json @@ -7,6 +7,6 @@ "postinstall": "./postinstall.sh" }, "devDependencies": { - "code-oss-dev": "coder/vscode#0fd21e4078ac1dddb26be024f5d4224a4b86da93" + "code-oss-dev": "coder/vscode#bd734e3d9f21b1bce4dabab2514177e90c090ee6" } } diff --git a/vendor/yarn.lock b/vendor/yarn.lock index abfb361b7..9a5eff2e1 100644 --- a/vendor/yarn.lock +++ b/vendor/yarn.lock @@ -274,9 +274,9 @@ clone-response@^1.0.2: dependencies: mimic-response "^1.0.0" -code-oss-dev@coder/vscode#0fd21e4078ac1dddb26be024f5d4224a4b86da93: +code-oss-dev@coder/vscode#bd734e3d9f21b1bce4dabab2514177e90c090ee6: version "1.63.0" - resolved "https://codeload.github.com/coder/vscode/tar.gz/0fd21e4078ac1dddb26be024f5d4224a4b86da93" + resolved "https://codeload.github.com/coder/vscode/tar.gz/bd734e3d9f21b1bce4dabab2514177e90c090ee6" dependencies: "@microsoft/applicationinsights-web" "^2.6.4" "@parcel/watcher" "2.0.3"