From 4b3abbe6760a8fa21ca7db20fb80fff3dea43bae Mon Sep 17 00:00:00 2001 From: Krist Wongsuphasawat Date: Tue, 13 Nov 2018 13:27:26 -0800 Subject: [PATCH] Add RegistryWithDefaultKey class (#27) * import main lodash instead of lodash/fp * add factory and unit test * more unit test to increase branch coverage * refactor color * update unit test and references --- .../src/CategoricalColorNamespace.js | 2 +- .../src/ColorSchemeRegistry.js | 50 +----- .../test/ColorSchemeRegistry.test.js | 95 +---------- .../packages/superset-ui-core/src/index.js | 1 + .../src/models/RegistryWithDefaultKey.js | 51 ++++++ .../src/utils/convertKeysToCamelCase.js | 8 +- .../superset-ui-core/test/index.test.js | 2 + .../test/models/Registry.test.js | 24 +++ .../models/RegistryWithDefaultKey.test.js | 154 ++++++++++++++++++ 9 files changed, 247 insertions(+), 140 deletions(-) create mode 100644 superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/models/RegistryWithDefaultKey.js create mode 100644 superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/models/RegistryWithDefaultKey.test.js diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/src/CategoricalColorNamespace.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/src/CategoricalColorNamespace.js index 6291da7909711..218085867cfe7 100644 --- a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/src/CategoricalColorNamespace.js +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/src/CategoricalColorNamespace.js @@ -9,7 +9,7 @@ export default class CategoricalColorNamespace { } getScale(schemeName) { - const name = schemeName || getCategoricalSchemeRegistry().getDefaultSchemeName(); + const name = schemeName || getCategoricalSchemeRegistry().getDefaultKey(); const scale = this.scales[name]; if (scale) { return scale; diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/src/ColorSchemeRegistry.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/src/ColorSchemeRegistry.js index d60de59c96a0c..1877615cc733e 100644 --- a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/src/ColorSchemeRegistry.js +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/src/ColorSchemeRegistry.js @@ -1,46 +1,10 @@ -import { Registry } from '@superset-ui/core'; +import { RegistryWithDefaultKey } from '@superset-ui/core'; -class ColorSchemeRegistry extends Registry { - clear() { - super.clear(); - this.defaultSchemeName = undefined; - - return this; - } - - getDefaultSchemeName() { - return this.defaultSchemeName; - } - - setDefaultSchemeName(schemeName) { - this.defaultSchemeName = schemeName; - - return this; - } - - get(schemeName) { - return super.get(schemeName || this.defaultSchemeName); - } - - registerValue(schemeName, colors) { - super.registerValue(schemeName, colors); - // If there is no default, set as default - if (!this.defaultSchemeName) { - this.defaultSchemeName = schemeName; - } - - return this; - } - - registerLoader(schemeName, loader) { - super.registerLoader(schemeName, loader); - // If there is no default, set as default - if (!this.defaultSchemeName) { - this.defaultSchemeName = schemeName; - } - - return this; +export default class ColorSchemeRegistry extends RegistryWithDefaultKey { + constructor() { + super({ + name: 'ColorScheme', + setFirstItemAsDefault: true, + }); } } - -export default ColorSchemeRegistry; diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/test/ColorSchemeRegistry.test.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/test/ColorSchemeRegistry.test.js index b46da44720600..b9cb4696ca941 100644 --- a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/test/ColorSchemeRegistry.test.js +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-color/test/ColorSchemeRegistry.test.js @@ -1,97 +1,8 @@ import ColorSchemeRegistry from '../src/ColorSchemeRegistry'; -import CategoricalScheme from '../src/CategoricalScheme'; describe('ColorSchemeRegistry', () => { - const registry = new ColorSchemeRegistry(); - const SCHEME1 = new CategoricalScheme({ - name: 'test', - colors: ['red', 'green', 'blue'], - }); - const SCHEME2 = new CategoricalScheme({ - name: 'test2', - colors: ['orange', 'yellow', 'pink'], - }); - const SCHEME3 = new CategoricalScheme({ - name: 'test3', - colors: ['cyan', 'magenta'], - }); - - beforeEach(() => { - registry.clear(); - registry.registerValue('test', SCHEME1); - registry.registerValue('test2', SCHEME2); - registry.setDefaultSchemeName('test'); - }); - describe('.clear()', () => { - it('clears the entries', () => { - registry.clear(); - expect(registry.has('test')).toBeFalsy(); - }); - it('clears default scheme name', () => { - registry.clear(); - expect(registry.getDefaultSchemeName()).toBeUndefined(); - }); - it('returns the ColorSchemeRegistry instance', () => { - const instance = registry.clear(); - expect(instance).toBe(registry); - }); - }); - describe('.get()', () => { - it('.get() returns default color scheme', () => { - const scheme = registry.get(); - expect(scheme).toEqual(SCHEME1); - }); - it('.get(name) returns color scheme with specified name', () => { - const scheme = registry.get('test2'); - expect(scheme).toEqual(SCHEME2); - }); - }); - describe('.getDefaultSchemeName()', () => { - it('returns default scheme name', () => { - const name = registry.getDefaultSchemeName(); - expect(name).toBe('test'); - }); - }); - describe('.setDefaultSchemeName()', () => { - it('set default scheme name', () => { - registry.setDefaultSchemeName('test2'); - const name = registry.getDefaultSchemeName(); - expect(name).toBe('test2'); - registry.setDefaultSchemeName('test'); - }); - it('returns the ColorSchemeRegistry instance', () => { - const instance = registry.setDefaultSchemeName('test'); - expect(instance).toBe(registry); - }); - }); - describe('.registerValue(name, colors)', () => { - it('sets schemename and color', () => { - registry.registerValue('test3', SCHEME3); - const scheme = registry.get('test3'); - expect(scheme).toEqual(SCHEME3); - }); - it('registers this scheme name as default if there is no default scheme', () => { - registry.clear().registerValue('test3', SCHEME3); - expect(registry.getDefaultSchemeName()).toEqual('test3'); - }); - it('returns the ColorSchemeRegistry instance', () => { - const instance = registry.registerValue('test3', SCHEME3); - expect(instance).toBe(registry); - }); - }); - describe('.registerLoader(name, colorsLoader)', () => { - it('sets schemename and color', () => { - registry.registerLoader('test3', () => SCHEME3); - const scheme = registry.get('test3'); - expect(scheme).toEqual(SCHEME3); - }); - it('registers this scheme name as default if there is no default scheme', () => { - registry.clear().registerLoader('test3', () => SCHEME3); - expect(registry.getDefaultSchemeName()).toEqual('test3'); - }); - it('returns the ColorSchemeRegistry instance', () => { - const instance = registry.registerLoader('test3', () => SCHEME3); - expect(instance).toBe(registry); - }); + it('exists', () => { + expect(ColorSchemeRegistry).toBeDefined(); + expect(ColorSchemeRegistry).toBeInstanceOf(Function); }); }); diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/index.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/index.js index 7d879d27ccc5f..20e2bc2d84dd1 100644 --- a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/index.js +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/index.js @@ -1,6 +1,7 @@ export { default as Plugin } from './models/Plugin'; export { default as Preset } from './models/Preset'; export { default as Registry } from './models/Registry'; +export { default as RegistryWithDefaultKey } from './models/RegistryWithDefaultKey'; export { default as convertKeysToCamelCase } from './utils/convertKeysToCamelCase'; export { default as isDefined } from './utils/isDefined'; diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/models/RegistryWithDefaultKey.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/models/RegistryWithDefaultKey.js new file mode 100644 index 0000000000000..a88d76624d80c --- /dev/null +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/models/RegistryWithDefaultKey.js @@ -0,0 +1,51 @@ +import Registry from './Registry'; + +export default class RegistryWithDefaultKey extends Registry { + constructor({ name, initialDefaultKey = undefined, setFirstItemAsDefault = false } = {}) { + super(name); + this.initialDefaultKey = initialDefaultKey; + this.defaultKey = initialDefaultKey; + this.setFirstItemAsDefault = setFirstItemAsDefault; + } + + clear() { + super.clear(); + this.defaultKey = this.initialDefaultKey; + + return this; + } + + get(key) { + return super.get(key || this.defaultKey); + } + + registerValue(key, value) { + super.registerValue(key, value); + // If there is no default, set as default + if (this.setFirstItemAsDefault && !this.defaultKey) { + this.defaultKey = key; + } + + return this; + } + + registerLoader(key, loader) { + super.registerLoader(key, loader); + // If there is no default, set as default + if (this.setFirstItemAsDefault && !this.defaultKey) { + this.defaultKey = key; + } + + return this; + } + + getDefaultKey() { + return this.defaultKey; + } + + setDefaultKey(key) { + this.defaultKey = key; + + return this; + } +} diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/utils/convertKeysToCamelCase.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/utils/convertKeysToCamelCase.js index ac99ce85b01c8..0be9d8b993eca 100644 --- a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/utils/convertKeysToCamelCase.js +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/utils/convertKeysToCamelCase.js @@ -1,13 +1,13 @@ -import camelCase from 'lodash/fp/camelCase'; -import isPlainObject from 'lodash/fp/isPlainObject'; -import mapKeys from 'lodash/fp/mapKeys'; +import camelCase from 'lodash/camelCase'; +import isPlainObject from 'lodash/isPlainObject'; +import mapKeys from 'lodash/mapKeys'; export default function convertKeysToCamelCase(object) { if (object === null || object === undefined) { return object; } if (isPlainObject(object)) { - return mapKeys(k => camelCase(k), object); + return mapKeys(object, (_, k) => camelCase(k)); } throw new Error(`Cannot convert input that is not a plain object: ${object}`); } diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/index.test.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/index.test.js index caafe5cc2a174..5e382d2981479 100644 --- a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/index.test.js +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/index.test.js @@ -2,6 +2,7 @@ import { Plugin, Preset, Registry, + RegistryWithDefaultKey, convertKeysToCamelCase, isDefined, isRequired, @@ -14,6 +15,7 @@ describe('index', () => { Plugin, Preset, Registry, + RegistryWithDefaultKey, convertKeysToCamelCase, isDefined, isRequired, diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/models/Registry.test.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/models/Registry.test.js index 0022478779702..83f3e0975809b 100644 --- a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/models/Registry.test.js +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/models/Registry.test.js @@ -52,6 +52,18 @@ describe('Registry', () => { expect(registry.has('a')).toBe(true); expect(registry.get('a')).toBe('testValue'); }); + it('does not overwrite if value is exactly the same', () => { + const registry = new Registry(); + const value = { a: 1 }; + registry.registerValue('a', value); + const promise1 = registry.getAsPromise('a'); + registry.registerValue('a', value); + const promise2 = registry.getAsPromise('a'); + expect(promise1).toBe(promise2); + registry.registerValue('a', { a: 1 }); + const promise3 = registry.getAsPromise('a'); + expect(promise1).not.toBe(promise3); + }); it('returns the registry itself', () => { const registry = new Registry(); expect(registry.registerValue('a', 'testValue')).toBe(registry); @@ -65,6 +77,18 @@ describe('Registry', () => { expect(registry.has('a')).toBe(true); expect(registry.get('a')).toBe('testValue'); }); + it('does not overwrite if loader is exactly the same', () => { + const registry = new Registry(); + const loader = () => 'testValue'; + registry.registerLoader('a', loader); + const promise1 = registry.getAsPromise('a'); + registry.registerLoader('a', loader); + const promise2 = registry.getAsPromise('a'); + expect(promise1).toBe(promise2); + registry.registerLoader('a', () => 'testValue'); + const promise3 = registry.getAsPromise('a'); + expect(promise1).not.toBe(promise3); + }); it('returns the registry itself', () => { const registry = new Registry(); expect(registry.registerLoader('a', () => 'testValue')).toBe(registry); diff --git a/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/models/RegistryWithDefaultKey.test.js b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/models/RegistryWithDefaultKey.test.js new file mode 100644 index 0000000000000..117d6aebf09b1 --- /dev/null +++ b/superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/models/RegistryWithDefaultKey.test.js @@ -0,0 +1,154 @@ +import RegistryWithDefaultKey from '../../src/models/RegistryWithDefaultKey'; +import Registry from '../../src/models/Registry'; + +describe('RegistryWithDefaultKey', () => { + it('exists', () => { + expect(RegistryWithDefaultKey).toBeDefined(); + }); + + let registry; + + beforeEach(() => { + registry = new RegistryWithDefaultKey(); + }); + + describe('new RegistryWithDefaultKey(options)', () => { + it('returns a class that extends from Registry', () => { + expect(registry).toBeInstanceOf(Registry); + }); + }); + + describe('.clear()', () => { + it('also resets default key', () => { + registry.setDefaultKey('abc'); + registry.clear(); + expect(registry.getDefaultKey()).toBeUndefined(); + }); + it('returns itself', () => { + expect(registry.clear()).toBe(registry); + }); + }); + + describe('.get()', () => { + beforeEach(() => { + registry + .registerValue('abc', 100) + .registerValue('def', 200) + .setDefaultKey('abc'); + }); + it('.get() returns value from default key', () => { + expect(registry.get()).toEqual(100); + }); + it('.get(key) returns value from specified key', () => { + expect(registry.get('def')).toEqual(200); + }); + }); + + describe('.getDefaultKey()', () => { + it('returns defaultKey', () => { + registry.setDefaultKey('abc'); + expect(registry.getDefaultKey()).toEqual('abc'); + }); + }); + + describe('.setDefaultKey(key)', () => { + it('set the default key', () => { + registry.setDefaultKey('abc'); + expect(registry.defaultKey).toEqual('abc'); + }); + it('returns itself', () => { + expect(registry.setDefaultKey('ghi')).toBe(registry); + }); + }); + + describe('options.defaultKey', () => { + describe('when not set', () => { + it(`After creation, default key is undefined`, () => { + expect(registry.defaultKey).toBeUndefined(); + }); + it('.clear() reset defaultKey to undefined', () => { + registry.setDefaultKey('abc'); + registry.clear(); + expect(registry.getDefaultKey()).toBeUndefined(); + }); + }); + describe('when options.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', () => { + registry2.setDefaultKey('abc'); + registry2.clear(); + expect(registry2.getDefaultKey()).toEqual('def'); + }); + }); + }); + + describe('options.setFirstItemAsDefault', () => { + describe('when true', () => { + const registry2 = new RegistryWithDefaultKey({ setFirstItemAsDefault: true }); + beforeEach(() => { + registry2.clear(); + }); + describe('.registerValue(key, value)', () => { + it('sets the default key to this key if default key is not set', () => { + registry2.registerValue('abc', 100); + expect(registry2.getDefaultKey()).toEqual('abc'); + }); + it('does not modify the default key if already set', () => { + registry2.setDefaultKey('def').registerValue('abc', 100); + expect(registry2.getDefaultKey()).toEqual('def'); + }); + it('returns itself', () => { + expect(registry2.registerValue('ghi', 300)).toBe(registry2); + }); + }); + describe('.registerLoader(key, loader)', () => { + it('sets the default key to this key if default key is not set', () => { + registry2.registerLoader('abc', () => 100); + expect(registry2.getDefaultKey()).toEqual('abc'); + }); + it('does not modify the default key if already set', () => { + registry2.setDefaultKey('def').registerLoader('abc', () => 100); + expect(registry2.getDefaultKey()).toEqual('def'); + }); + it('returns itself', () => { + expect(registry2.registerLoader('ghi', () => 300)).toBe(registry2); + }); + }); + }); + describe('when false', () => { + const registry2 = new RegistryWithDefaultKey({ setFirstItemAsDefault: false }); + beforeEach(() => { + registry2.clear(); + }); + describe('.registerValue(key, value)', () => { + it('does not modify default key', () => { + registry2.registerValue('abc', 100); + expect(registry2.defaultKey).toBeUndefined(); + registry2.setDefaultKey('def'); + registry2.registerValue('ghi', 300); + expect(registry2.defaultKey).toEqual('def'); + }); + it('returns itself', () => { + expect(registry2.registerValue('ghi', 300)).toBe(registry2); + }); + }); + describe('.registerLoader(key, loader)', () => { + it('does not modify default key', () => { + registry2.registerValue('abc', () => 100); + expect(registry2.defaultKey).toBeUndefined(); + registry2.setDefaultKey('def'); + registry2.registerValue('ghi', () => 300); + expect(registry2.defaultKey).toEqual('def'); + }); + it('returns itself', () => { + expect(registry2.registerLoader('ghi', () => 300)).toBe(registry2); + }); + }); + }); + }); +});