Merge pull request #1758 from yuvipanda/urlsearchparamsagain

JS: Add unit tests (with coverage) & use `URL` object instead of strings to construct some URLs

.eslintrc.js

@@ -2,12 +2,16 @@ module.exports = {
     "env": {
         "browser": true,
         "jquery": true,
-        "es6": true
+        "es6": true,
+        "jest/globals": true
     },
     "extends": "eslint:recommended",
     "parserOptions": {
         "sourceType": "module"
     },
     "rules": {
-    }
+    },
+    "plugins": [
+        "jest"
+    ]
 };

.github/workflows/jest.yml

@@ -0,0 +1,36 @@
+# Runs jest based unit tests for frontend javascript and @jupyterhub/binderhub-client
+name: "JS Unit tests"
+
+on:
+  pull_request:
+    paths:
+      - "binderhub/static/js/**"
+      - "js/packages/binderhub-client/**"
+      - ".github/workflows/jest.yml"
+  push:
+    paths:
+      - "binderhub/static/js/**"
+      - "js/packages/binderhub-client/**"
+      - ".github/workflows/jest.yml"
+    branches-ignore:
+      - "dependabot/**"
+      - "pre-commit-ci-update-config"
+      - "update-*"
+  workflow_dispatch:
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: "Setup dependencies"
+        run: |
+          npm install
+
+      - name: "Run all unit tests"
+        run: |
+          npm test
+
+      - name: Upload coverage to Codecov
+        uses: codecov/codecov-action@v3

binderhub/static/js/index.js

@@ -19,6 +19,7 @@ import { BinderRepository } from '@jupyterhub/binderhub-client';
 import { makeBadgeMarkup } from './src/badge';
 import { getPathType, updatePathText } from './src/path';
 import { nextHelpText } from './src/loading';
+import { updateFavicon } from './src/favicon';
 
 import 'xterm/css/xterm.css';
 
@@ -33,14 +34,6 @@ const BASE_URL = $('#base-url').data().url;
 const BADGE_BASE_URL = $('#badge-base-url').data().url;
 let config_dict = {};
 
-function update_favicon(path) {
-    let link = document.querySelector("link[rel*='icon']") || document.createElement('link');
-    link.type = 'image/x-icon';
-    link.rel = 'shortcut icon';
-    link.href = path;
-    document.getElementsByTagName('head')[0].appendChild(link);
-}
-
 function v2url(providerPrefix, repository, ref, path, pathType) {
   // return a v2 url from a providerPrefix, repository, ref, and (file|url)path
   if (repository.length === 0) {
@@ -153,7 +146,7 @@ function updateUrls(formValues) {
 }
 
 function build(providerSpec, log, fitAddon, path, pathType) {
-  update_favicon(BASE_URL + "favicon_building.ico");
+  updateFavicon(BASE_URL + "favicon_building.ico");
   // split provider prefix off of providerSpec
   const spec = providerSpec.slice(providerSpec.indexOf('/') + 1);
   // Update the text of the loading page if it exists
@@ -167,7 +160,11 @@ function build(providerSpec, log, fitAddon, path, pathType) {
   $('.on-build').removeClass('hidden');
 
   const buildToken = $("#build-token").data('token');
-  const image = new BinderRepository(providerSpec, BASE_URL, buildToken);
+  // If BASE_URL is absolute, use that as the base for build endpoint URL.
+  // Else, first resolve BASE_URL relative to current URL, then use *that* as the
+  // base for the build endpoint url.
+  const buildEndpointUrl = new URL("build", new URL(BASE_URL, window.location.href));
+  const image = new BinderRepository(providerSpec, buildEndpointUrl, buildToken);
 
   image.onStateChange('*', function(oldState, newState, data) {
     if (data.message !== undefined) {
@@ -198,7 +195,7 @@ function build(providerSpec, log, fitAddon, path, pathType) {
     $("#loader").addClass("paused");
 
     // If we fail for any reason, show an error message and logs
-    update_favicon(BASE_URL + "favicon_fail.ico");
+    updateFavicon(BASE_URL + "favicon_fail.ico");
     log.show();
     if ($('div#loader-text').length > 0) {
       $('#loader').addClass("error");
@@ -213,13 +210,16 @@ function build(providerSpec, log, fitAddon, path, pathType) {
       $('#phase-launching').removeClass('hidden');
     }
     $('#phase-launching').removeClass('hidden');
-    update_favicon(BASE_URL + "favicon_success.ico");
+    updateFavicon(BASE_URL + "favicon_success.ico");
   });
 
   image.onStateChange('ready', function(oldState, newState, data) {
     image.close();
+    // If data.url is an absolute URL, it'll be used. Else, it'll be interpreted
+    // relative to current page's URL.
+    const serverUrl = new URL(data.url, window.location.href);
     // user server is ready, redirect to there
-    image.launch(data.url, data.token, path, pathType);
+    window.location.href = image.getFullRedirectURL(serverUrl, data.token, path, pathType);
   });
 
   image.fetch();

binderhub/static/js/src/favicon.js

@@ -0,0 +1,17 @@
+/**
+ * Dynamically set current page's favicon.
+ *
+ * @param {String} href Path to Favicon to use
+ */
+function updateFavicon(href) {
+    let link = document.querySelector("link[rel*='icon']");
+    if(!link) {
+        link = document.createElement('link');
+        document.getElementsByTagName('head')[0].appendChild(link);
+    }
+    link.type = 'image/x-icon';
+    link.rel = 'shortcut icon';
+    link.href = href;
+}
+
+export { updateFavicon };

binderhub/static/js/src/favicon.test.js

@@ -0,0 +1,29 @@
+import { updateFavicon } from "./favicon";
+
+afterEach(() => {
+    // Clear out HEAD after each test run, so our DOM is clean.
+    // Jest does *not* clear out the DOM between test runs on the same file!
+    document.querySelector("head").innerHTML = '';
+});
+
+test("Setting favicon when there is none works", () => {
+    expect(document.querySelector("link[rel*='icon']")).toBeNull();
+
+    updateFavicon("https://example.com/somefile.png");
+
+    expect(document.querySelector("link[rel*='icon']").href).toBe("https://example.com/somefile.png");
+});
+
+test("Setting favicon multiple times works without leaking link tags", () => {
+    expect(document.querySelector("link[rel*='icon']")).toBeNull();
+
+    updateFavicon("https://example.com/somefile.png");
+
+    expect(document.querySelector("link[rel*='icon']").href).toBe("https://example.com/somefile.png");
+    expect(document.querySelectorAll("link[rel*='icon']").length).toBe(1);
+
+    updateFavicon("https://example.com/some-other-file.png");
+
+    expect(document.querySelector("link[rel*='icon']").href).toBe("https://example.com/some-other-file.png");
+    expect(document.querySelectorAll("link[rel*='icon']").length).toBe(1);
+});

js/packages/binderhub-client/lib/index.js

@@ -10,13 +10,28 @@ export class BinderRepository {
   /**
    *
    * @param {string} providerSpec Spec of the form <provider>/<repo>/<ref> to pass to the binderhub API.
-   * @param {string} baseUrl Base URL (including the trailing slash) of the binderhub installation to talk to.
-   * @param {string} buildToken Optional JWT based build token if this binderhub installation requires using build tokesn
+   * @param {URL} buildEndpointUrl API URL of the build endpoint to talk to
+   * @param {string} buildToken Optional JWT based build token if this binderhub installation requires using build tokens
    */
-  constructor(providerSpec, baseUrl, buildToken) {
+  constructor(providerSpec, buildEndpointUrl, buildToken) {
     this.providerSpec = providerSpec;
-    this.baseUrl = baseUrl;
-    this.buildToken = buildToken;
+    // Make sure that buildEndpointUrl is a real URL - this ensures hostname is properly set
+    if(!(buildEndpointUrl instanceof URL)) {
+      throw new TypeError(`buildEndpointUrl must be a URL object, got ${buildEndpointUrl} instead`);
+    }
+    // We make a copy here so we don't modify the passed in URL object
+    this.buildEndpointUrl = new URL(buildEndpointUrl);
+    // The binderHub API is path based, so the buildEndpointUrl must have a trailing slash. We add
+    // it if it is not passed in here to us.
+    if(!this.buildEndpointUrl.pathname.endsWith('/')) {
+      this.buildEndpointUrl.pathname += "/";
+    }
+
+    // The actual URL we'll make a request to build this particular providerSpec
+    this.buildUrl = new URL(this.providerSpec, this.buildEndpointUrl);
+    if(buildToken) {
+      this.buildUrl.searchParams.append("build_token", buildToken);
+    }
     this.callbacks = {};
     this.state = null;
   }
@@ -25,12 +40,7 @@ export class BinderRepository {
    * Call the BinderHub API
    */
   fetch() {
-    let apiUrl = this.baseUrl + "build/" + this.providerSpec;
-    if (this.buildToken) {
-        apiUrl = apiUrl + `?build_token=${this.buildToken}`;
-    }
-
-    this.eventSource = new EventSource(apiUrl);
+    this.eventSource = new EventSource(this.buildUrl);
     this.eventSource.onerror = (err) => {
       console.error("Failed to construct event stream", err);
       this._changeState("failed", {
@@ -59,38 +69,48 @@ export class BinderRepository {
   }
 
   /**
-   * Redirect user to a running jupyter server with given token
+   * Get URL to redirect user to on a Jupyter Server to display a given path
 
-   * @param {URL} url URL to the running jupyter server
+   * @param {URL} serverUrl URL to the running jupyter server
    * @param {string} token Secret token used to authenticate to the jupyter server
    * @param {string} path The path of the file or url suffix to launch the user into
    * @param {string} pathType One of "lab", "file" or "url", denoting what kinda path we are launching the user into
+   *
+   * @returns {URL} A URL to redirect the user to
    */
-  launch(url, token, path, pathType) {
-    // redirect a user to a running server with a token
+  getFullRedirectURL(serverUrl, token, path, pathType) {
+    // Make a copy of the URL so we don't mangle the original
+    let url = new URL(serverUrl);
     if (path) {
-      // strip trailing /
-      url = url.replace(/\/$/, "");
-      // trim leading '/'
+      // strip trailing / from URL
+      url.pathname = url.pathname.replace(/\/$/, "");
+
+      // trim leading '/' from path to launch users into
       path = path.replace(/(^\/)/g, "");
+
       if (pathType === "lab") {
+        // The path is a specific *file* we should open with JupyterLab
+
         // trim trailing / on file paths
         path = path.replace(/(\/$)/g, "");
+
         // /doc/tree is safe because it allows redirect to files
-        url = url + "/doc/tree/" + encodeURI(path);
+        url.pathname = url.pathname + "/doc/tree/" + encodeURI(path);
       } else if (pathType === "file") {
+        // The path is a specific file we should open with *classic notebook*
+
         // trim trailing / on file paths
         path = path.replace(/(\/$)/g, "");
         // /tree is safe because it allows redirect to files
-        url = url + "/tree/" + encodeURI(path);
+        url.pathname = url.pathname + "/tree/" + encodeURI(path);
       } else {
-        // pathType === 'url'
-        url = url + "/" + path;
+        // pathType is 'url' and we should just pass it on
+        url.pathname = url.pathname + "/" + path;
       }
     }
-    const sep = url.indexOf("?") == -1 ? "?" : "&";
-    url = url + sep + $.param({ token: token });
-    window.location.href = url;
+
+    url.searchParams.append('token', token);
+    return url;
   }