Skip to content

Commit 042a624

Browse files
committed
Cache options parsing
This way it can be called multiple times without incurring extra overhead or potentially resolving in a different way. Also catch any errors when parsing query options just as we do with the options in the markup and log errors with the latter as well.
1 parent 2690e22 commit 042a624

File tree

3 files changed

+89
-37
lines changed

3 files changed

+89
-37
lines changed

src/common/util.ts

+28-19
Original file line numberDiff line numberDiff line change
@@ -66,33 +66,42 @@ export const resolveBase = (base?: string): string => {
6666
return normalize(url.pathname)
6767
}
6868

69+
let options: Options
70+
6971
/**
7072
* Get options embedded in the HTML or query params.
7173
*/
7274
export const getOptions = <T extends Options>(): T => {
73-
let options: T
74-
try {
75-
options = JSON.parse(document.getElementById("coder-options")!.getAttribute("data-settings")!)
76-
} catch (error) {
77-
options = {} as T
78-
}
75+
if (!options) {
76+
try {
77+
options = JSON.parse(document.getElementById("coder-options")!.getAttribute("data-settings")!)
78+
} catch (error) {
79+
console.error(error)
80+
options = {} as T
81+
}
7982

80-
// You can also pass options in stringified form to the options query
81-
// variable. Options provided here will override the ones in the options
82-
// element.
83-
const params = new URLSearchParams(location.search)
84-
const queryOpts = params.get("options")
85-
if (queryOpts) {
86-
options = {
87-
...options,
88-
...JSON.parse(queryOpts),
83+
// You can also pass options in stringified form to the options query
84+
// variable. Options provided here will override the ones in the options
85+
// element.
86+
const params = new URLSearchParams(location.search)
87+
const queryOpts = params.get("options")
88+
if (queryOpts) {
89+
try {
90+
options = {
91+
...options,
92+
...JSON.parse(queryOpts),
93+
}
94+
} catch (error) {
95+
// Don't fail if the query parameters are malformed.
96+
console.error(error)
97+
}
8998
}
90-
}
9199

92-
options.base = resolveBase(options.base)
93-
options.csStaticBase = resolveBase(options.csStaticBase)
100+
options.base = resolveBase(options.base)
101+
options.csStaticBase = resolveBase(options.csStaticBase)
102+
}
94103

95-
return options
104+
return options as T
96105
}
97106

98107
/**

test/unit/common/util.test.ts

+45-17
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { JSDOM } from "jsdom"
22
import * as util from "../../../src/common/util"
3-
import { createLoggerMock } from "../../utils/helpers"
3+
import * as helpers from "../../utils/helpers"
44

55
const dom = new JSDOM()
66
global.document = dom.window.document
@@ -111,7 +111,15 @@ describe("util", () => {
111111
})
112112

113113
describe("getOptions", () => {
114+
let getOptions: typeof import("../../src/common/util").getOptions
115+
114116
beforeEach(() => {
117+
// Reset and re-import since the options are cached.
118+
jest.resetModules()
119+
getOptions = require("../../src/common/util").getOptions
120+
121+
helpers.spyOnConsole()
122+
115123
const location: LocationLike = {
116124
pathname: "/healthz",
117125
origin: "http://localhost:8080",
@@ -135,43 +143,63 @@ describe("util", () => {
135143
base: "",
136144
csStaticBase: "",
137145
})
146+
expect(console.error).toBeCalledTimes(1)
138147
})
139148

140149
it("should return options when they do exist", () => {
150+
const expected = {
151+
base: ".",
152+
csStaticBase: "./static/development/Users/jp/Dev/code-server",
153+
logLevel: 2,
154+
disableTelemetry: false,
155+
disableUpdateCheck: false,
156+
}
157+
141158
// Mock getElementById
142159
const spy = jest.spyOn(document, "getElementById")
143-
// Create a fake element and set the attribute
160+
// Create a fake element and set the attribute. Options are expected to be
161+
// stringified JSON.
144162
const mockElement = document.createElement("div")
145-
mockElement.setAttribute(
146-
"data-settings",
147-
'{"base":".","csStaticBase":"./static/development/Users/jp/Dev/code-server","logLevel":2,"disableUpdateCheck":false}',
148-
)
163+
mockElement.setAttribute("data-settings", JSON.stringify(expected))
149164
// Return mockElement from the spy
150165
// this way, when we call "getElementById"
151166
// it returns the element
152167
spy.mockImplementation(() => mockElement)
153168

154169
expect(util.getOptions()).toStrictEqual({
170+
...expected,
171+
// The two bases should get resolved. The rest should be unchanged.
155172
base: "",
156173
csStaticBase: "/static/development/Users/jp/Dev/code-server",
157-
disableUpdateCheck: false,
174+
})
175+
expect(console.error).toBeCalledTimes(0)
176+
})
177+
178+
it("should merge options in the query", () => {
179+
// Options provided in the query will override any options provided in the
180+
// HTML. Options are expected to be stringified JSON (same as the
181+
// element).
182+
const expected = {
158183
logLevel: 2,
184+
}
185+
location.search = `?options=${JSON.stringify(expected)}`
186+
expect(getOptions()).toStrictEqual({
187+
...expected,
188+
base: "",
189+
csStaticBase: "",
159190
})
191+
// Once for the element.
192+
expect(console.error).toBeCalledTimes(1)
160193
})
161194

162-
it("should include queryOpts", () => {
163-
// Trying to understand how the implementation works
164-
// 1. It grabs the search params from location.search (i.e. ?)
165-
// 2. it then grabs the "options" param if it exists
166-
// 3. then it creates a new options object
167-
// spreads the original options
168-
// then parses the queryOpts
169-
location.search = '?options={"logLevel":2}'
195+
it("should skip bad query options", () => {
196+
location.search = "?options=invalidJson"
170197
expect(util.getOptions()).toStrictEqual({
171198
base: "",
172199
csStaticBase: "",
173-
logLevel: 2,
174200
})
201+
// Once for the element, once for the query.
202+
expect(console.error).toBeCalledTimes(2)
175203
})
176204
})
177205

@@ -217,7 +245,7 @@ describe("util", () => {
217245
jest.restoreAllMocks()
218246
})
219247

220-
const loggerModule = createLoggerMock()
248+
const loggerModule = helpers.createLoggerMock()
221249

222250
it("should log an error with the message and stack trace", () => {
223251
const message = "You don't have access to that folder."

test/utils/helpers.ts

+16-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { promises as fs } from "fs"
2+
import * as net from "net"
23
import * as os from "os"
34
import * as path from "path"
4-
import * as net from "net"
55

66
/**
77
* Return a mock of @coder/logger.
@@ -29,8 +29,23 @@ export async function clean(testName: string): Promise<void> {
2929
await fs.rmdir(dir, { recursive: true })
3030
}
3131

32+
/**
33+
* Spy on the console and surpress its output.
34+
* TODO: Replace `createLoggerMock` usage with `spyOnConsole`.
35+
*/
36+
export function spyOnConsole() {
37+
const noop = () => undefined
38+
jest.spyOn(global.console, "debug").mockImplementation(noop)
39+
jest.spyOn(global.console, "error").mockImplementation(noop)
40+
jest.spyOn(global.console, "info").mockImplementation(noop)
41+
jest.spyOn(global.console, "trace").mockImplementation(noop)
42+
jest.spyOn(global.console, "warn").mockImplementation(noop)
43+
}
44+
3245
/**
3346
* Create a uniquely named temporary directory for a test.
47+
*
48+
* These directories are placed under a single temporary code-server directory.
3449
*/
3550
export async function tmpdir(testName: string): Promise<string> {
3651
const dir = path.join(os.tmpdir(), `code-server/tests/${testName}`)

0 commit comments

Comments
 (0)