Clean up handling of default urls (#5111)

* Clean up handling of wsUrl

* Formatting

* Simplify normalization logic

* Add normalize function

* Use url normalize for wsUrl as well

* Clean up error messages

* Clean up error messages

* Clean up logic

* Update comment

* Actually return the settings

* Clean up handling of urls in makeSettings

* clean up comment

* cleanup:

* Fix logic error

* Update handling of baseUrl and add notes

* Switch to location.origin

* Normalize window.location.origin

* Simplify logic

* test

* test

* test

* Revert husky changes

* Allow wsUrl to be optional

packages/coreutils/src/pageconfig.ts

@@ -112,17 +112,10 @@ export namespace PageConfig {
   }
 
   /**
-   * Get the base url for a Jupyter application.
+   * Get the base url for a Jupyter application, or the base url of the page.
    */
   export function getBaseUrl(): string {
-    let baseUrl = getOption('baseUrl');
-    if (!baseUrl || baseUrl === '/') {
-      baseUrl =
-        typeof location === 'undefined'
-          ? 'http://localhost:8888/'
-          : location.origin + '/';
-    }
-    return URLExt.parse(baseUrl).toString();
+    return URLExt.normalize(getOption('baseUrl') || '/');
   }
 
   /**
@@ -143,22 +136,18 @@ export namespace PageConfig {
   }
 
   /**
-   * Get the base websocket url for a Jupyter application.
+   * Get the base websocket url for a Jupyter application, or an empty string.
    */
   export function getWsUrl(baseUrl?: string): string {
     let wsUrl = getOption('wsUrl');
     if (!wsUrl) {
-      baseUrl = baseUrl || getBaseUrl();
+      baseUrl = baseUrl ? URLExt.normalize(baseUrl) : getBaseUrl();
       if (baseUrl.indexOf('http') !== 0) {
-        if (typeof location !== 'undefined') {
-          baseUrl = URLExt.join(location.origin, baseUrl);
-        } else {
-          baseUrl = URLExt.join('http://localhost:8888/', baseUrl);
-        }
+        return '';
       }
       wsUrl = 'ws' + baseUrl.slice(4);
     }
-    return URLExt.parse(wsUrl).toString();
+    return URLExt.normalize(wsUrl);
   }
 
   /**

packages/coreutils/src/url.ts

@@ -25,6 +25,13 @@ export namespace URLExt {
     return urlparse(url);
   }
 
+  /**
+   * Normalize a url.
+   */
+  export function normalize(url: string): string {
+    return url && parse(url).toString();
+  }
+
   /**
    * Join a sequence of url components and normalizes as in node `path.join`.
    *

packages/services/src/serverconnection.ts

@@ -199,27 +199,24 @@ namespace Private {
   export function makeSettings(
     options: Partial<ServerConnection.ISettings> = {}
   ): ServerConnection.ISettings {
-    let extra: Partial<ServerConnection.ISettings> = {};
-    if (options.baseUrl && !options.wsUrl) {
-      // Setting baseUrl to https://host... sets wsUrl to wss://host...
-      let baseUrl = options.baseUrl;
-      if (baseUrl.indexOf('http') !== 0) {
-        if (typeof location !== 'undefined') {
-          baseUrl = URLExt.join(location.origin, baseUrl);
-        } else {
-          // TODO: Why are we hardcoding localhost and 8888? That doesn't seem
-          // good. See https://github.com/jupyterlab/jupyterlab/issues/4761
-          baseUrl = URLExt.join('http://localhost:8888/', baseUrl);
-        }
-      }
-      extra = {
-        wsUrl: 'ws' + baseUrl.slice(4)
-      };
+    const defaultSettings = ServerConnection.defaultSettings;
+    const baseUrl =
+      URLExt.normalize(options.baseUrl) || defaultSettings.baseUrl;
+    let wsUrl = options.wsUrl;
+    // Prefer the default wsUrl if we are using the default baseUrl.
+    if (!wsUrl && baseUrl === defaultSettings.baseUrl) {
+      wsUrl = defaultSettings.wsUrl;
+    }
+    // Otherwise convert the baseUrl to a wsUrl if possible.
+    if (!wsUrl && baseUrl.indexOf('http') === 0) {
+      wsUrl = 'ws' + baseUrl.slice(4);
     }
+    // Otherwise fall back on the default wsUrl.
+    wsUrl = wsUrl || defaultSettings.wsUrl;
     return {
-      ...ServerConnection.defaultSettings,
+      ...defaultSettings,
       ...options,
-      ...extra
+      ...{ wsUrl }
     };
   }
 

tests/test-coreutils/src/pageconfig.spec.ts

@@ -45,7 +45,18 @@ describe('@jupyterlab/coreutils', () => {
     describe('#getWsUrl()', () => {
       it('should get the base ws url of the page', () => {
         // The value was passed as a command line arg.
-        expect(PageConfig.getWsUrl()).to.contain('ws://localhost');
+        const expected = 'ws' + PageConfig.getBaseUrl().slice(4);
+        expect(PageConfig.getWsUrl()).to.equal(expected);
+      });
+
+      it('should handle a good base url', () => {
+        const url = 'http://foo.com';
+        expect(PageConfig.getWsUrl(url)).to.equal('ws://foo.com/');
+      });
+
+      it('should be an empty string for a bad base url', () => {
+        const url = 'blargh://foo.com';
+        expect(PageConfig.getWsUrl(url)).to.equal('');
       });
     });
   });

tests/test-coreutils/src/url.spec.ts

@@ -56,6 +56,26 @@ describe('@jupyterlab/coreutils', () => {
       });
     });
 
+    describe('.normalize()', () => {
+      it('should handle leading slash', () => {
+        expect(URLExt.normalize('/')).to.equal(location.origin + '/');
+      });
+
+      it('should handle leading double slash', () => {
+        expect(URLExt.normalize('//foo')).to.equal(
+          location.protocol + '//foo/'
+        );
+      });
+
+      it('should handle http', () => {
+        expect(URLExt.normalize('http://foo')).to.equal('http://foo/');
+      });
+
+      it('should handle other', () => {
+        expect(URLExt.normalize('ftp://foo')).to.equal('ftp://foo/');
+      });
+    });
+
     describe('objectToQueryString()', () => {
       it('should return a serialized object string suitable for a query', () => {
         const obj = {