fix: correct STORAGE_EMULATOR_HOST handling (#2069, #1314) (#2070)

* fix: correct STORAGE_EMULATOR_HOST handling (#2069, #1314)

credit to @jpambrun for identifying the fix

* fix: normalize baseUrl

Co-authored-by: Daniel Bankhead <dan@danielbankhead.com>

* fix: adjust URL normalization & tests for consistency

Co-authored-by: Daniel Bankhead <dan@danielbankhead.com>

src/storage.ts

@@ -664,8 +664,12 @@ export class Storage extends Service {
 
     options = Object.assign({}, options, {apiEndpoint});
 
-    // Note: EMULATOR_HOST is an experimental configuration variable. Use apiEndpoint instead.
-    const baseUrl = EMULATOR_HOST || `${options.apiEndpoint}/storage/v1`;
+    // Note: EMULATOR_HOST, if present and not overridden, has been applied to
+    // `options` at this point. Also, this uses string concatenation because the
+    // endpoint may contain a base path, and any trailing slash on that will
+    // have been removed, so using the two-arg URL constructor for relative path
+    // resolution won't work.
+    const baseUrl = new URL(options.apiEndpoint + '/storage/v1').href;
 
     const config = {
       apiEndpoint: options.apiEndpoint!,

test/index.ts

@@ -435,27 +435,33 @@ describe('Storage', () => {
         delete process.env.STORAGE_EMULATOR_HOST;
       });
 
-      it('should set baseUrl to env var STORAGE_EMULATOR_HOST', () => {
+      it('should set baseUrl to env var STORAGE_EMULATOR_HOST plus standard path', () => {
         const storage = new Storage({
           projectId: PROJECT_ID,
         });
 
         const calledWith = storage.calledWith_[0];
-        assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST);
+        assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST + '/storage/v1');
         assert.strictEqual(
           calledWith.apiEndpoint,
           'https://internal.benchmark.com/path'
         );
       });
 
-      it('should be overriden by apiEndpoint', () => {
+      it('should be overridden by apiEndpoint', () => {
         const storage = new Storage({
           projectId: PROJECT_ID,
           apiEndpoint: 'https://some.api.com',
         });
 
         const calledWith = storage.calledWith_[0];
-        assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST);
+        // NOTE: this used to assert partially the opposite of what the test
+        // says: it checked that baseUrl was _not_ overridden, but apiEndpoint
+        // was.
+        assert.strictEqual(
+          calledWith.baseUrl,
+          'https://some.api.com/storage/v1'
+        );
         assert.strictEqual(calledWith.apiEndpoint, 'https://some.api.com');
       });
 
@@ -468,7 +474,13 @@ describe('Storage', () => {
         });
 
         const calledWith = storage.calledWith_[0];
-        assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST);
+        // NOTE: this used to assert partially the opposite of what the test
+        // says: it checked that baseUrl was _not_ overridden, but apiEndpoint
+        // was.
+        assert.strictEqual(
+          calledWith.baseUrl,
+          'https://internal.benchmark.com/path/storage/v1'
+        );
         assert.strictEqual(
           calledWith.apiEndpoint,
           'https://internal.benchmark.com/path'