Replace several uses of `console.warn` with `warnOnce`

This reduces warning spam in the event where the code path that triggers
the warning is invoked repeatedly and there is no value to the developer
in telling them about the issue more than once.

To make `warnOnce` an easier drop-in replacement for `console.warn`, it
has been modified to accept multiple arguments, all of which get
forwarded to `console.warn`.

src/annotator/features.js

 'use strict';
 
 const events = require('../shared/bridge-events');
+const warnOnce = require('../shared/warn-once');
 
 let _features = {};
 
@@ -19,7 +20,7 @@ module.exports = {
 
   flagEnabled: function(flag) {
     if (!(flag in _features)) {
-      console.warn('looked up unknown feature', flag);
+      warnOnce('looked up unknown feature', flag);
       return false;
     }
     return _features[flag];

src/annotator/test/features-test.js

@@ -5,6 +5,8 @@ const features = require('../features');
 
 describe('features - annotation layer', function() {
   let featureFlagsUpdateHandler;
+  let fakeWarnOnce;
+
   const initialFeatures = {
     feature_on: true,
     feature_off: false,
@@ -15,7 +17,10 @@ describe('features - annotation layer', function() {
   };
 
   beforeEach(function() {
-    sinon.stub(console, 'warn');
+    fakeWarnOnce = sinon.stub();
+    features.$imports.$mock({
+      '../shared/warn-once': fakeWarnOnce,
+    });
 
     features.init({
       on: function(topic, handler) {
@@ -30,8 +35,8 @@ describe('features - annotation layer', function() {
   });
 
   afterEach(function() {
-    console.warn.restore();
     features.reset();
+    features.$imports.$restore();
   });
 
   describe('flagEnabled', function() {
@@ -51,10 +56,10 @@ describe('features - annotation layer', function() {
     });
 
     it('should warn when accessing unknown flags', function() {
-      assert.notCalled(console.warn);
+      assert.notCalled(fakeWarnOnce);
       assert.isFalse(features.flagEnabled('unknown_feature'));
-      assert.calledOnce(console.warn);
-      assert.calledWith(console.warn, 'looked up unknown feature');
+      assert.calledOnce(fakeWarnOnce);
+      assert.calledWith(fakeWarnOnce, 'looked up unknown feature');
     });
   });
 });

src/sidebar/util/test/warn-once-test.js → src/shared/test/warn-once-test.js

@@ -23,4 +23,15 @@ describe('warnOnce', () => {
     warnOnce('something else is wrong');
     assert.calledWith(console.warn, 'something else is wrong');
   });
+
+  it('supports multiple arguments', () => {
+    warnOnce('foo', 'bar', 'baz');
+    assert.calledWith(console.warn, 'foo', 'bar', 'baz');
+  });
+
+  it('supports non-string arguments', () => {
+    const obj = {};
+    warnOnce(1, {}, false);
+    assert.calledWith(console.warn, 1, obj, false);
+  });
 });

src/sidebar/util/warn-once.js → src/shared/warn-once.js

@@ -8,14 +8,18 @@ let shownWarnings = {};
  * This is useful to avoid spamming the console if a warning is emitted in a
  * context that may be called frequently.
  *
- * @param {string} warning
+ * @param {...any} args -
+ *   Arguments to forward to `console.warn`. The arguments `toString()` values
+ *   are concatenated into a string key which is used to determine if the warning
+ *   has been logged before.
  */
-function warnOnce(warning) {
-  if (warning in shownWarnings) {
+function warnOnce(...args) {
+  const key = args.join();
+  if (key in shownWarnings) {
     return;
   }
-  console.warn(warning);
-  shownWarnings[warning] = true;
+  console.warn(...args);
+  shownWarnings[key] = true;
 }
 
 warnOnce.reset = () => {

src/sidebar/services/features.js

@@ -13,9 +13,10 @@
 
 const events = require('../events');
 const bridgeEvents = require('../../shared/bridge-events');
+const warnOnce = require('../../shared/warn-once');
 
 // @ngInject
-function features($log, $rootScope, bridge, session) {
+function features($rootScope, bridge, session) {
   const _sendFeatureFlags = function() {
     const userFeatures = session.state.features;
     bridge.call(bridgeEvents.FEATURE_FLAGS_UPDATED, userFeatures || {});
@@ -50,7 +51,7 @@ function features($log, $rootScope, bridge, session) {
 
     const features = session.state.features;
     if (!(flag in features)) {
-      $log.warn('looked up unknown feature', flag);
+      warnOnce('looked up unknown feature', flag);
       return false;
     }
     return features[flag];

src/sidebar/services/streamer.js

@@ -3,6 +3,8 @@
 const queryString = require('query-string');
 const uuid = require('node-uuid');
 
+const warnOnce = require('../../shared/warn-once');
+
 const events = require('../events');
 const Socket = require('../websocket');
 
@@ -103,7 +105,7 @@ function Streamer(
   }
 
   function handleSocketOnError(event) {
-    console.warn('Error connecting to H push notification service:', event);
+    warnOnce('Error connecting to H push notification service:', event);
 
     // In development, warn if the connection failure might be due to
     // the app's origin not having been whitelisted in the H service's config.
@@ -112,7 +114,7 @@ function Streamer(
     // HTTP status code for HTTP -> WS upgrade requests.
     const websocketHost = new URL(settings.websocketUrl).hostname;
     if (['localhost', '127.0.0.1'].indexOf(websocketHost) !== -1) {
-      console.warn(
+      warnOnce(
         'Check that your H service is configured to allow ' +
           'WebSocket connections from ' +
           window.location.origin
@@ -144,7 +146,7 @@ function Streamer(
           );
         }
       } else {
-        console.warn('received unsupported notification', message.type);
+        warnOnce('received unsupported notification', message.type);
       }
     });
   }

src/sidebar/services/test/features-test.js

@@ -6,7 +6,7 @@ const bridgeEvents = require('../../../shared/bridge-events');
 
 describe('h:features - sidebar layer', function() {
   let fakeBridge;
-  let fakeLog;
+  let fakeWarnOnce;
   let fakeRootScope;
   let fakeSession;
   let sandbox;
@@ -18,9 +18,7 @@ describe('h:features - sidebar layer', function() {
       call: sinon.stub(),
     };
 
-    fakeLog = {
-      warn: sinon.stub(),
-    };
+    fakeWarnOnce = sinon.stub();
 
     fakeRootScope = {
       eventCallbacks: {},
@@ -41,54 +39,39 @@ describe('h:features - sidebar layer', function() {
         },
       },
     };
+
+    features.$imports.$mock({
+      '../../shared/warn-once': fakeWarnOnce,
+    });
   });
 
   afterEach(function() {
+    features.$imports.$restore();
     sandbox.restore();
   });
 
   describe('flagEnabled', function() {
     it('should retrieve features data', function() {
-      const features_ = features(
-        fakeLog,
-        fakeRootScope,
-        fakeBridge,
-        fakeSession
-      );
+      const features_ = features(fakeRootScope, fakeBridge, fakeSession);
       assert.equal(features_.flagEnabled('feature_on'), true);
       assert.equal(features_.flagEnabled('feature_off'), false);
     });
 
     it('should return false if features have not been loaded', function() {
-      const features_ = features(
-        fakeLog,
-        fakeRootScope,
-        fakeBridge,
-        fakeSession
-      );
+      const features_ = features(fakeRootScope, fakeBridge, fakeSession);
       // simulate feature data not having been loaded yet
       fakeSession.state = {};
       assert.equal(features_.flagEnabled('feature_on'), false);
     });
 
     it('should trigger a refresh of session data', function() {
-      const features_ = features(
-        fakeLog,
-        fakeRootScope,
-        fakeBridge,
-        fakeSession
-      );
+      const features_ = features(fakeRootScope, fakeBridge, fakeSession);
       features_.flagEnabled('feature_on');
       assert.calledOnce(fakeSession.load);
     });
 
     it('should return false for unknown flags', function() {
-      const features_ = features(
-        fakeLog,
-        fakeRootScope,
-        fakeBridge,
-        fakeSession
-      );
+      const features_ = features(fakeRootScope, fakeBridge, fakeSession);
       assert.isFalse(features_.flagEnabled('unknown_feature'));
     });
   });
@@ -97,7 +80,7 @@ describe('h:features - sidebar layer', function() {
     assert.notProperty(fakeRootScope.eventCallbacks, events.USER_CHANGED);
     assert.notProperty(fakeRootScope.eventCallbacks, events.FRAME_CONNECTED);
 
-    features(fakeLog, fakeRootScope, fakeBridge, fakeSession);
+    features(fakeRootScope, fakeBridge, fakeSession);
 
     assert.property(fakeRootScope.eventCallbacks, events.USER_CHANGED);
     assert.property(fakeRootScope.eventCallbacks, events.FRAME_CONNECTED);

src/sidebar/store/test/use-store-test.js

@@ -82,7 +82,7 @@ describe('useStore', () => {
   it('warns if the callback always returns a different value', () => {
     const warnOnce = sinon.stub();
     $imports.$mock({
-      '../util/warn-once': warnOnce,
+      '../../shared/warn-once': warnOnce,
     });
     const BuggyComponent = () => {
       // The result of the callback is an object with an `aValue` property

src/sidebar/store/use-store.js

@@ -6,7 +6,7 @@ const shallowEqual = require('shallowequal');
 const { useEffect, useRef, useReducer } = require('preact/hooks');
 
 const { useService } = require('../util/service-context');
-const warnOnce = require('../util/warn-once');
+const warnOnce = require('../../shared/warn-once');
 
 /**
  * Hook for accessing state or actions from the store inside a component.