Merge pull request #1151 from hypothesis/warn-less

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

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,9 @@ describe('features - annotation layer', function() {
     });
 
     it('should warn when accessing unknown flags', function() {
-      assert.notCalled(console.warn);
       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.