[#1281] Limit obsolete decorator to 5 messages

Closes #1281
excaliburjs · Oct 9, 2019 · af13bed · af13bed
1 parent f7edf9c
commit af13bed
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Changed
 
+- Changed obsolete decorator to only log the same message 5 times. ([#1281](https://github.com/excaliburjs/Excalibur/issues/1281))
+
 ### Deprecated
 
 ### Removed

diff --git a/src/engine/Util/Decorators.ts b/src/engine/Util/Decorators.ts
@@ -13,6 +13,27 @@ export interface ObsoleteOptions {
   showStackTrace?: boolean;
 }
 
+export const maxMessages = 5;
+let obsoleteMessage: { [messageCount: string]: number } = {};
+export const resetObsoleteCounter = () => {
+  for (const message in obsoleteMessage) {
+    obsoleteMessage[message] = 0;
+  }
+};
+
+const logMessage = (message: string, options: ObsoleteOptions) => {
+  if (obsoleteMessage[message] < maxMessages) {
+    Logger.getInstance().warn(message);
+
+    // tslint:disable-next-line: no-console
+    if (console.trace && options.showStackTrace) {
+      // tslint:disable-next-line: no-console
+      console.trace();
+    }
+  }
+  obsoleteMessage[message]++;
+};
+
 /**
  * Obsolete decorator for marking Excalibur methods obsolete, you can optionally specify a custom message and/or alternate replacement
  * method do the deprecated one. Inspired by https://github.com/jayphelps/core-decorators.js
@@ -41,17 +62,16 @@ export function obsolete(options?: ObsoleteOptions): any {
       `${methodSignature} is marked obsolete: ${options.message}` +
       (options.alternateMethod ? ` Use ${options.alternateMethod} instead` : '');
 
+    if (!obsoleteMessage[message]) {
+      obsoleteMessage[message] = 0;
+    }
+
     // If descriptor is null it is a class
     const method = descriptor ? { ...descriptor } : target;
     if (!descriptor) {
       const constructor = function() {
         const args = Array.prototype.slice.call(arguments);
-        Logger.getInstance().warn(message);
-        // tslint:disable-next-line: no-console
-        if (console.trace && options.showStackTrace) {
-          // tslint:disable-next-line: no-console
-          console.trace();
-        }
+        logMessage(message, options);
         return new method(...args);
       };
       constructor.prototype = method.prototype;
@@ -60,32 +80,22 @@ export function obsolete(options?: ObsoleteOptions): any {
 
     if (descriptor && descriptor.value) {
       method.value = function(this: any) {
-        Logger.getInstance().warn(message);
-        // tslint:disable-next-line: no-console
-        if (console.trace && options.showStackTrace) {
-          // tslint:disable-next-line: no-console
-          console.trace();
-        }
+        logMessage(message, options);
         return descriptor.value.apply(this, arguments);
       };
       return method;
     }
 
     if (descriptor && descriptor.get) {
       method.get = function(this: any) {
-        Logger.getInstance().warn(message);
-        // tslint:disable-next-line: no-console
-        if (console.trace && options.showStackTrace) {
-          // tslint:disable-next-line: no-console
-          console.trace();
-        }
+        logMessage(message, options);
         return descriptor.get.apply(this, arguments);
       };
     }
 
     if (descriptor && descriptor.set) {
       method.set = function(this: any) {
-        Logger.getInstance().warn(message);
+        logMessage(message, options);
         return descriptor.set.apply(this, arguments);
       };
     }

diff --git a/src/spec/DecoratorSpec.ts b/src/spec/DecoratorSpec.ts
@@ -40,6 +40,11 @@ describe('An @obsolete decorator', () => {
     logger = ex.Logger.getInstance();
     spyOn(logger, 'warn');
   });
+
+  afterEach(() => {
+    ex.resetObsoleteCounter();
+  });
+
   it('exists', () => {
     expect(ex.obsolete).toBeDefined();
   });
@@ -82,4 +87,32 @@ describe('An @obsolete decorator', () => {
       'ObsoleteClass is marked obsolete: This feature will be ' + 'removed in future versions of Excalibur.'
     );
   });
+
+  it('is rate limited on method', () => {
+    for (let i = 0; i < 10; i++) {
+      testObsolete.method();
+    }
+    expect(logger.warn).toHaveBeenCalledTimes(5);
+  });
+
+  it('is rate limited on setter', () => {
+    for (let i = 0; i < 10; i++) {
+      testObsolete.setter = 'stuff';
+    }
+    expect(logger.warn).toHaveBeenCalledTimes(5);
+  });
+
+  it('is rate limited on getter', () => {
+    for (let i = 0; i < 10; i++) {
+      let value = testObsolete.getter;
+    }
+    expect(logger.warn).toHaveBeenCalledTimes(5);
+  });
+
+  it('is rate limited on classes', () => {
+    for (let i = 0; i < 10; i++) {
+      let value = new ObsoleteClass();
+    }
+    expect(logger.warn).toHaveBeenCalledTimes(5);
+  });
 });