Cache options parsing

code-asher · code-asher · commit 5ca8625cec38 · 2021-09-27T20:06:38.000Z
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.
diff --git a/src/common/util.ts b/src/common/util.ts
@@ -66,33 +66,42 @@ export const resolveBase = (base?: string): string => {
   return normalize(url.pathname)
 }
 
+let options: Options
+
 /**
  * Get options embedded in the HTML or query params.
  */
 export const getOptions = <T extends Options>(): T => {
-  let options: T
-  try {
-    options = JSON.parse(document.getElementById("coder-options")!.getAttribute("data-settings")!)
-  } catch (error) {
-    options = {} as T
-  }
+  if (!options) {
+    try {
+      options = JSON.parse(document.getElementById("coder-options")!.getAttribute("data-settings")!)
+    } catch (error) {
+      console.error(error)
+      options = {} as T
+    }
 
-  // You can also pass options in stringified form to the options query
-  // variable. Options provided here will override the ones in the options
-  // element.
-  const params = new URLSearchParams(location.search)
-  const queryOpts = params.get("options")
-  if (queryOpts) {
-    options = {
-      ...options,
-      ...JSON.parse(queryOpts),
+    // You can also pass options in stringified form to the options query
+    // variable. Options provided here will override the ones in the options
+    // element.
+    const params = new URLSearchParams(location.search)
+    const queryOpts = params.get("options")
+    if (queryOpts) {
+      try {
+        options = {
+          ...options,
+          ...JSON.parse(queryOpts),
+        }
+      } catch (error) {
+        // Don't fail if the query parameters are malformed.
+        console.error(error)
+      }
     }
-  }
 
-  options.base = resolveBase(options.base)
-  options.csStaticBase = resolveBase(options.csStaticBase)
+    options.base = resolveBase(options.base)
+    options.csStaticBase = resolveBase(options.csStaticBase)
+  }
 
-  return options
+  return options as T
 }
 
 /**
diff --git a/test/unit/common/util.test.ts b/test/unit/common/util.test.ts
@@ -1,6 +1,6 @@
 import { JSDOM } from "jsdom"
 import * as util from "../../../src/common/util"
-import { createLoggerMock } from "../../utils/helpers"
+import * as helpers from "../../utils/helpers"
 
 const dom = new JSDOM()
 global.document = dom.window.document
@@ -111,6 +111,8 @@ describe("util", () => {
   })
 
   describe("getOptions", () => {
+    let getOptions: typeof import("../../../src/common/util").getOptions
+
     beforeEach(() => {
       const location: LocationLike = {
         pathname: "/healthz",
@@ -124,54 +126,80 @@ describe("util", () => {
       // and tell TS that our location should be looked at
       // as Location (even though it's missing some properties)
       global.location = location as Location
+
+      // Reset and re-import since the options are cached.
+      jest.resetModules()
+      getOptions = require("../../../src/common/util").getOptions
+
+      helpers.spyOnConsole()
     })
 
     afterEach(() => {
       jest.restoreAllMocks()
     })
 
     it("should return options with base and cssStaticBase even if it doesn't exist", () => {
-      expect(util.getOptions()).toStrictEqual({
+      expect(getOptions()).toStrictEqual({
         base: "",
         csStaticBase: "",
       })
+      expect(console.error).toBeCalledTimes(1)
     })
 
     it("should return options when they do exist", () => {
+      const expected = {
+        base: ".",
+        csStaticBase: "./static/development/Users/jp/Dev/code-server",
+        logLevel: 2,
+        disableTelemetry: false,
+        disableUpdateCheck: false,
+      }
+
       // Mock getElementById
       const spy = jest.spyOn(document, "getElementById")
-      // Create a fake element and set the attribute
+      // Create a fake element and set the attribute. Options are expected to be
+      // stringified JSON.
       const mockElement = document.createElement("div")
-      mockElement.setAttribute(
-        "data-settings",
-        '{"base":".","csStaticBase":"./static/development/Users/jp/Dev/code-server","logLevel":2,"disableUpdateCheck":false}',
-      )
+      mockElement.setAttribute("data-settings", JSON.stringify(expected))
       // Return mockElement from the spy
       // this way, when we call "getElementById"
       // it returns the element
       spy.mockImplementation(() => mockElement)
 
-      expect(util.getOptions()).toStrictEqual({
+      expect(getOptions()).toStrictEqual({
+        ...expected,
+        // The two bases should get resolved. The rest should be unchanged.
         base: "",
         csStaticBase: "/static/development/Users/jp/Dev/code-server",
-        disableUpdateCheck: false,
+      })
+      expect(console.error).toBeCalledTimes(0)
+    })
+
+    it("should merge options in the query", () => {
+      // Options provided in the query will override any options provided in the
+      // HTML. Options are expected to be stringified JSON (same as the
+      // element).
+      const expected = {
         logLevel: 2,
+      }
+      location.search = `?options=${JSON.stringify(expected)}`
+      expect(getOptions()).toStrictEqual({
+        ...expected,
+        base: "",
+        csStaticBase: "",
       })
+      // Once for the element.
+      expect(console.error).toBeCalledTimes(1)
     })
 
-    it("should include queryOpts", () => {
-      // Trying to understand how the implementation works
-      // 1. It grabs the search params from location.search (i.e. ?)
-      // 2. it then grabs the "options" param if it exists
-      // 3. then it creates a new options object
-      // spreads the original options
-      // then parses the queryOpts
-      location.search = '?options={"logLevel":2}'
-      expect(util.getOptions()).toStrictEqual({
+    it("should skip bad query options", () => {
+      location.search = "?options=invalidJson"
+      expect(getOptions()).toStrictEqual({
         base: "",
         csStaticBase: "",
-        logLevel: 2,
       })
+      // Once for the element, once for the query.
+      expect(console.error).toBeCalledTimes(2)
     })
   })
 
@@ -217,7 +245,7 @@ describe("util", () => {
       jest.restoreAllMocks()
     })
 
-    const loggerModule = createLoggerMock()
+    const loggerModule = helpers.createLoggerMock()
 
     it("should log an error with the message and stack trace", () => {
       const message = "You don't have access to that folder."
diff --git a/test/utils/helpers.ts b/test/utils/helpers.ts
@@ -29,8 +29,23 @@ export async function clean(testName: string): Promise<void> {
   await fs.rmdir(dir, { recursive: true })
 }
 
+/**
+ * Spy on the console and surpress its output.
+ * TODO: Replace `createLoggerMock` usage with `spyOnConsole`.
+ */
+export function spyOnConsole() {
+  const noop = () => undefined
+  jest.spyOn(global.console, "debug").mockImplementation(noop)
+  jest.spyOn(global.console, "error").mockImplementation(noop)
+  jest.spyOn(global.console, "info").mockImplementation(noop)
+  jest.spyOn(global.console, "trace").mockImplementation(noop)
+  jest.spyOn(global.console, "warn").mockImplementation(noop)
+}
+
 /**
  * Create a uniquely named temporary directory for a test.
+ *
+ * These directories are placed under a single temporary code-server directory.
  */
 export async function tmpdir(testName: string): Promise<string> {
   const dir = path.join(os.tmpdir(), `code-server/tests/${testName}`)
