Skip to content

Commit c70183a

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 cd339d4 commit c70183a

File tree

3 files changed

+84
-37
lines changed

3 files changed

+84
-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/util.test.ts

+44-18
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,14 @@ import {
33
arrayify,
44
generateUuid,
55
getFirstString,
6-
getOptions,
76
logError,
87
plural,
98
resolveBase,
109
split,
1110
trimSlashes,
1211
normalize,
1312
} from "../../src/common/util"
14-
import { createLoggerMock } from "../utils/helpers"
13+
import { createLoggerMock, spyOnConsole } from "../utils/helpers"
1514

1615
const dom = new JSDOM()
1716
global.document = dom.window.document
@@ -122,7 +121,15 @@ describe("util", () => {
122121
})
123122

124123
describe("getOptions", () => {
124+
let getOptions: typeof import("../../src/common/util").getOptions
125+
125126
beforeEach(() => {
127+
// Reset and re-import since the options are cached.
128+
jest.resetModules()
129+
getOptions = require("../../src/common/util").getOptions
130+
131+
spyOnConsole()
132+
126133
const location: LocationLike = {
127134
pathname: "/healthz",
128135
origin: "http://localhost:8080",
@@ -146,44 +153,63 @@ describe("util", () => {
146153
base: "",
147154
csStaticBase: "",
148155
})
156+
expect(console.error).toBeCalledTimes(1)
149157
})
150158

151159
it("should return options when they do exist", () => {
160+
const expected = {
161+
base: ".",
162+
csStaticBase: "./static/development/Users/jp/Dev/code-server",
163+
logLevel: 2,
164+
disableTelemetry: false,
165+
disableUpdateCheck: false,
166+
}
167+
152168
// Mock getElementById
153169
const spy = jest.spyOn(document, "getElementById")
154-
// Create a fake element and set the attribute
170+
// Create a fake element and set the attribute. Options are expected to be
171+
// stringified JSON.
155172
const mockElement = document.createElement("div")
156-
mockElement.setAttribute(
157-
"data-settings",
158-
'{"base":".","csStaticBase":"./static/development/Users/jp/Dev/code-server","logLevel":2,"disableTelemetry":false,"disableUpdateCheck":false}',
159-
)
173+
mockElement.setAttribute("data-settings", JSON.stringify(expected))
160174
// Return mockElement from the spy
161175
// this way, when we call "getElementById"
162176
// it returns the element
163177
spy.mockImplementation(() => mockElement)
164178

165179
expect(getOptions()).toStrictEqual({
180+
...expected,
181+
// The two bases should get resolved. The rest should be unchanged.
166182
base: "",
167183
csStaticBase: "/static/development/Users/jp/Dev/code-server",
168-
disableTelemetry: false,
169-
disableUpdateCheck: false,
184+
})
185+
expect(console.error).toBeCalledTimes(0)
186+
})
187+
188+
it("should merge options in the query", () => {
189+
// Options provided in the query will override any options provided in the
190+
// HTML. Options are expected to be stringified JSON (same as the
191+
// element).
192+
const expected = {
170193
logLevel: 2,
194+
}
195+
location.search = `?options=${JSON.stringify(expected)}`
196+
expect(getOptions()).toStrictEqual({
197+
...expected,
198+
base: "",
199+
csStaticBase: "",
171200
})
201+
// Once for the element.
202+
expect(console.error).toBeCalledTimes(1)
172203
})
173204

174-
it("should include queryOpts", () => {
175-
// Trying to understand how the implementation works
176-
// 1. It grabs the search params from location.search (i.e. ?)
177-
// 2. it then grabs the "options" param if it exists
178-
// 3. then it creates a new options object
179-
// spreads the original options
180-
// then parses the queryOpts
181-
location.search = '?options={"logLevel":2}'
205+
it("should skip bad query options", () => {
206+
location.search = "?options=invalidJson"
182207
expect(getOptions()).toStrictEqual({
183208
base: "",
184209
csStaticBase: "",
185-
logLevel: 2,
186210
})
211+
// Once for the element, once for the query.
212+
expect(console.error).toBeCalledTimes(2)
187213
})
188214
})
189215

test/utils/helpers.ts

+12
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,18 @@ export function createLoggerMock() {
1919
}
2020
}
2121

22+
/**
23+
* Spy on console and surpress its output.
24+
*/
25+
export function spyOnConsole() {
26+
const noop = () => undefined
27+
jest.spyOn(global.console, "debug").mockImplementation(noop)
28+
jest.spyOn(global.console, "error").mockImplementation(noop)
29+
jest.spyOn(global.console, "info").mockImplementation(noop)
30+
jest.spyOn(global.console, "trace").mockImplementation(noop)
31+
jest.spyOn(global.console, "warn").mockImplementation(noop)
32+
}
33+
2234
/**
2335
* Create a uniquely named temporary directory.
2436
*

0 commit comments

Comments
 (0)