Add overwritePolicy for Registry (apache#37)

BREAKING CHANGE: Change Registry constructor API to take object instead of single string name. feat: Add overwritePolicy for Registry so developer can customize whether overwriting is ALLOW, WARN or PROHIBIT.
graceguo-supercat · Nov 19, 2018 · d4bdc78 · d4bdc78
1 parent 7072c8b
commit d4bdc78
Show file tree

Hide file tree

Showing 9 changed files with 121 additions and 16 deletions.
diff --git a/packages/superset-ui-chart/src/registries/ChartBuildQueryRegistrySingleton.js b/packages/superset-ui-chart/src/registries/ChartBuildQueryRegistrySingleton.js
@@ -2,7 +2,7 @@ import { Registry, makeSingleton } from '@superset-ui/core';
 
 class ChartBuildQueryRegistry extends Registry {
   constructor() {
-    super('ChartBuildQuery');
+    super({ name: 'ChartBuildQuery' });
   }
 }
 

diff --git a/packages/superset-ui-chart/src/registries/ChartComponentRegistrySingleton.js b/packages/superset-ui-chart/src/registries/ChartComponentRegistrySingleton.js
@@ -2,7 +2,7 @@ import { Registry, makeSingleton } from '@superset-ui/core';
 
 class ChartComponentRegistry extends Registry {
   constructor() {
-    super('ChartComponent');
+    super({ name: 'ChartComponent' });
   }
 }
 

diff --git a/packages/superset-ui-chart/src/registries/ChartMetadataRegistrySingleton.js b/packages/superset-ui-chart/src/registries/ChartMetadataRegistrySingleton.js
@@ -2,7 +2,7 @@ import { Registry, makeSingleton } from '@superset-ui/core';
 
 class ChartMetadataRegistry extends Registry {
   constructor() {
-    super('ChartMetadata');
+    super({ name: 'ChartMetadata' });
   }
 }
 

diff --git a/packages/superset-ui-chart/src/registries/ChartTransformPropsRegistrySingleton.js b/packages/superset-ui-chart/src/registries/ChartTransformPropsRegistrySingleton.js
@@ -2,7 +2,7 @@ import { Registry, makeSingleton } from '@superset-ui/core';
 
 class ChartTransformPropsRegistry extends Registry {
   constructor() {
-    super('ChartTransformProps');
+    super({ name: 'ChartTransformProps' });
   }
 }
 

diff --git a/packages/superset-ui-core/package.json b/packages/superset-ui-core/package.json
@@ -25,6 +25,9 @@
   "publishConfig": {
     "access": "public"
   },
+  "devDependencies": {
+    "jest-mock-console": "^0.4.0"
+  },
   "dependencies": {
     "lodash": "^4.17.11"
   }

diff --git a/packages/superset-ui-core/src/models/Registry.js b/packages/superset-ui-core/src/models/Registry.js
@@ -1,5 +1,14 @@
+/* eslint no-console: 0 */
+
+const OverwritePolicy = {
+  ALLOW: 'ALLOW',
+  PROHIBIT: 'PROHIBIT',
+  WARN: 'WARN',
+};
+
 export default class Registry {
-  constructor(name = '') {
+  constructor({ name = '', overwritePolicy = OverwritePolicy.ALLOW } = {}) {
+    this.overwritePolicy = overwritePolicy;
     this.name = name;
     this.items = {};
     this.promises = {};
@@ -20,6 +29,13 @@ export default class Registry {
 
   registerValue(key, value) {
     const item = this.items[key];
+    if (item && item.value !== value) {
+      if (this.overwritePolicy === OverwritePolicy.WARN) {
+        console.warn(`Item with key "${key}" already exists. You are assigning a new value.`);
+      } else if (this.overwritePolicy === OverwritePolicy.PROHIBIT) {
+        throw new Error(`Item with key "${key}" already exists. Cannot overwrite.`);
+      }
+    }
     if (!item || item.value !== value) {
       this.items[key] = { value };
       delete this.promises[key];
@@ -30,6 +46,13 @@ export default class Registry {
 
   registerLoader(key, loader) {
     const item = this.items[key];
+    if (item && item.loader !== loader) {
+      if (this.overwritePolicy === OverwritePolicy.WARN) {
+        console.warn(`Item with key "${key}" already exists. You are assigning a new value.`);
+      } else if (this.overwritePolicy === OverwritePolicy.PROHIBIT) {
+        throw new Error(`Item with key "${key}" already exists. Cannot overwrite.`);
+      }
+    }
     if (!item || item.loader !== loader) {
       this.items[key] = { loader };
       delete this.promises[key];
@@ -122,3 +145,5 @@ export default class Registry {
     return this;
   }
 }
+
+Registry.OverwritePolicy = OverwritePolicy;
diff --git a/packages/superset-ui-core/src/models/RegistryWithDefaultKey.js b/packages/superset-ui-core/src/models/RegistryWithDefaultKey.js
@@ -1,8 +1,8 @@
 import Registry from './Registry';
 
 export default class RegistryWithDefaultKey extends Registry {
-  constructor({ name, initialDefaultKey = undefined, setFirstItemAsDefault = false } = {}) {
-    super(name);
+  constructor({ initialDefaultKey = undefined, setFirstItemAsDefault = false, ...rest } = {}) {
+    super(rest);
     this.initialDefaultKey = initialDefaultKey;
     this.defaultKey = initialDefaultKey;
     this.setFirstItemAsDefault = setFirstItemAsDefault;

diff --git a/packages/superset-ui-core/test/models/Registry.test.js b/packages/superset-ui-core/test/models/Registry.test.js
@@ -1,17 +1,19 @@
+/* eslint no-console: 0 */
+import mockConsole from 'jest-mock-console';
 import Registry from '../../src/models/Registry';
 
 describe('Registry', () => {
   it('exists', () => {
     expect(Registry !== undefined).toBe(true);
   });
 
-  describe('new Registry(name)', () => {
-    it('can create a new registry when name is not given', () => {
+  describe('new Registry(config)', () => {
+    it('can create a new registry when config.name is not given', () => {
       const registry = new Registry();
       expect(registry).toBeInstanceOf(Registry);
     });
-    it('can create a new registry when name is given', () => {
-      const registry = new Registry('abc');
+    it('can create a new registry when config.name is given', () => {
+      const registry = new Registry({ name: 'abc' });
       expect(registry).toBeInstanceOf(Registry);
       expect(registry.name).toBe('abc');
     });
@@ -265,4 +267,79 @@ describe('Registry', () => {
       expect(registry.remove('a')).toBe(registry);
     });
   });
+
+  describe('config.overwritePolicy', () => {
+    describe('=ALLOW', () => {
+      describe('.registerValue(key, value)', () => {
+        it('registers normally', () => {
+          const restoreConsole = mockConsole();
+          const registry = new Registry();
+          registry.registerValue('a', 'testValue');
+          expect(() => registry.registerValue('a', 'testValue2')).not.toThrow();
+          expect(registry.get('a')).toEqual('testValue2');
+          expect(console.warn).not.toHaveBeenCalled();
+          restoreConsole();
+        });
+      });
+      describe('.registerLoader(key, loader)', () => {
+        it('registers normally', () => {
+          const restoreConsole = mockConsole();
+          const registry = new Registry();
+          registry.registerLoader('a', () => 'testValue');
+          expect(() => registry.registerLoader('a', () => 'testValue2')).not.toThrow();
+          expect(registry.get('a')).toEqual('testValue2');
+          expect(console.warn).not.toHaveBeenCalled();
+          restoreConsole();
+        });
+      });
+    });
+    describe('=WARN', () => {
+      describe('.registerValue(key, value)', () => {
+        it('warns when overwrite', () => {
+          const restoreConsole = mockConsole();
+          const registry = new Registry({
+            overwritePolicy: Registry.OverwritePolicy.WARN,
+          });
+          registry.registerValue('a', 'testValue');
+          expect(() => registry.registerValue('a', 'testValue2')).not.toThrow();
+          expect(registry.get('a')).toEqual('testValue2');
+          expect(console.warn).toHaveBeenCalled();
+          restoreConsole();
+        });
+      });
+      describe('.registerLoader(key, loader)', () => {
+        it('warns when overwrite', () => {
+          const restoreConsole = mockConsole();
+          const registry = new Registry({
+            overwritePolicy: Registry.OverwritePolicy.WARN,
+          });
+          registry.registerLoader('a', () => 'testValue');
+          expect(() => registry.registerLoader('a', () => 'testValue2')).not.toThrow();
+          expect(registry.get('a')).toEqual('testValue2');
+          expect(console.warn).toHaveBeenCalled();
+          restoreConsole();
+        });
+      });
+    });
+    describe('=PROHIBIT', () => {
+      describe('.registerValue(key, value)', () => {
+        it('throws error when overwrite', () => {
+          const registry = new Registry({
+            overwritePolicy: Registry.OverwritePolicy.PROHIBIT,
+          });
+          registry.registerValue('a', 'testValue');
+          expect(() => registry.registerValue('a', 'testValue2')).toThrow();
+        });
+      });
+      describe('.registerLoader(key, loader)', () => {
+        it('warns when overwrite', () => {
+          const registry = new Registry({
+            overwritePolicy: Registry.OverwritePolicy.PROHIBIT,
+          });
+          registry.registerLoader('a', () => 'testValue');
+          expect(() => registry.registerLoader('a', () => 'testValue2')).toThrow();
+        });
+      });
+    });
+  });
 });
diff --git a/packages/superset-ui-core/test/models/RegistryWithDefaultKey.test.js b/packages/superset-ui-core/test/models/RegistryWithDefaultKey.test.js
@@ -12,7 +12,7 @@ describe('RegistryWithDefaultKey', () => {
     registry = new RegistryWithDefaultKey();
   });
 
-  describe('new RegistryWithDefaultKey(options)', () => {
+  describe('new RegistryWithDefaultKey(config)', () => {
     it('returns a class that extends from Registry', () => {
       expect(registry).toBeInstanceOf(Registry);
     });
@@ -61,7 +61,7 @@ describe('RegistryWithDefaultKey', () => {
     });
   });
 
-  describe('options.defaultKey', () => {
+  describe('config.defaultKey', () => {
     describe('when not set', () => {
       it(`After creation, default key is undefined`, () => {
         expect(registry.defaultKey).toBeUndefined();
@@ -72,22 +72,22 @@ describe('RegistryWithDefaultKey', () => {
         expect(registry.getDefaultKey()).toBeUndefined();
       });
     });
-    describe('when options.initialDefaultKey is set', () => {
+    describe('when config.initialDefaultKey is set', () => {
       const registry2 = new RegistryWithDefaultKey({
         initialDefaultKey: 'def',
       });
       it(`After creation, default key is undefined`, () => {
         expect(registry2.defaultKey).toEqual('def');
       });
-      it('.clear() reset defaultKey to this options.defaultKey', () => {
+      it('.clear() reset defaultKey to this config.defaultKey', () => {
         registry2.setDefaultKey('abc');
         registry2.clear();
         expect(registry2.getDefaultKey()).toEqual('def');
       });
     });
   });
 
-  describe('options.setFirstItemAsDefault', () => {
+  describe('config.setFirstItemAsDefault', () => {
     describe('when true', () => {
       const registry2 = new RegistryWithDefaultKey({ setFirstItemAsDefault: true });
       beforeEach(() => {