-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add data structures for chart plugin system #6028
Merged
Merged
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
de4e716
add unit tests
kristw ed7a53a
add test structure
kristw 608e160
add unit tests for Registry
kristw 1f496d4
add LoaderRegistry unit test
kristw 92a22ff
add unit test for makeSingleton
kristw 5d1a757
add type check
kristw 288513d
add plugin data structures
kristw dfae36c
simplify API
kristw 585f3f2
add preset tests
kristw 36e1017
update test message
kristw 932bbf4
fix lint
kristw 047e2d7
update makeSingleton
kristw 1043d4e
update Plugin, Preset and unit test
kristw 428146f
revise Registry code
kristw de72995
update unit test, add remove function
kristw 58ad704
update test
kristw 0904e4a
update unit test
kristw 0deeecb
update plugin unit test
kristw 3f8fff6
add .keys(), .entries() and .entriesAsPromise()
kristw 18eae1b
update test description
kristw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
132 changes: 132 additions & 0 deletions
132
superset/assets/spec/javascripts/modules/Registry_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
import { describe, it } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import Registry from '../../../src/modules/Registry'; | ||
|
||
describe('Registry', () => { | ||
it('exists', () => { | ||
expect(Registry !== undefined).to.equal(true); | ||
}); | ||
|
||
describe('new Registry(name)', () => { | ||
it('can create a new registry when name is not given', () => { | ||
const registry = new Registry(); | ||
expect(registry).to.be.instanceOf(Registry); | ||
}); | ||
it('can create a new registry when name is given', () => { | ||
const registry = new Registry('abc'); | ||
expect(registry).to.be.instanceOf(Registry); | ||
expect(registry.name).to.equal('abc'); | ||
}); | ||
}); | ||
describe('.has(key)', () => { | ||
it('returns true if an item with the given key exists', () => { | ||
const registry = new Registry(); | ||
registry.registerValue('a', 'testValue'); | ||
expect(registry.has('a')).to.equal(true); | ||
registry.registerLoader('b', () => 'testValue2'); | ||
expect(registry.has('b')).to.equal(true); | ||
}); | ||
it('returns false if an item with the given key does not exist', () => { | ||
const registry = new Registry(); | ||
expect(registry.has('a')).to.equal(false); | ||
}); | ||
}); | ||
describe('.registerValue(key, value)', () => { | ||
it('registers the given value with the given key', () => { | ||
const registry = new Registry(); | ||
registry.registerValue('a', 'testValue'); | ||
expect(registry.has('a')).to.equal(true); | ||
expect(registry.get('a')).to.equal('testValue'); | ||
}); | ||
it('returns the registry itself', () => { | ||
const registry = new Registry(); | ||
expect(registry.registerValue('a', 'testValue')).to.equal(registry); | ||
}); | ||
}); | ||
describe('.registerLoader(key, loader)', () => { | ||
it('registers the given loader with the given key', () => { | ||
const registry = new Registry(); | ||
registry.registerLoader('a', () => 'testValue'); | ||
expect(registry.has('a')).to.equal(true); | ||
expect(registry.get('a')).to.equal('testValue'); | ||
}); | ||
it('returns the registry itself', () => { | ||
const registry = new Registry(); | ||
expect(registry.registerLoader('a', () => 'testValue')).to.equal(registry); | ||
}); | ||
}); | ||
describe('.get(key)', () => { | ||
it('given the key, returns the value if the item is a value', () => { | ||
const registry = new Registry(); | ||
registry.registerValue('a', 'testValue'); | ||
expect(registry.get('a')).to.equal('testValue'); | ||
}); | ||
it('given the key, returns the result of the loader function if the item is a loader', () => { | ||
const registry = new Registry(); | ||
registry.registerLoader('b', () => 'testValue2'); | ||
expect(registry.get('b')).to.equal('testValue2'); | ||
}); | ||
it('returns null if the item with specified key does not exist', () => { | ||
const registry = new Registry(); | ||
expect(registry.get('a')).to.equal(null); | ||
}); | ||
it('If the key was registered multiple times, returns the most recent item.', () => { | ||
const registry = new Registry(); | ||
registry.registerValue('a', 'testValue'); | ||
expect(registry.get('a')).to.equal('testValue'); | ||
registry.registerLoader('a', () => 'newValue'); | ||
expect(registry.get('a')).to.equal('newValue'); | ||
}); | ||
}); | ||
describe('.getAsPromise(key)', () => { | ||
it('given the key, returns a promise of item value if the item is a value', () => { | ||
const registry = new Registry(); | ||
registry.registerValue('a', 'testValue'); | ||
return registry.getAsPromise('a').then((value) => { | ||
expect(value).to.equal('testValue'); | ||
}); | ||
}); | ||
it('given the key, returns a promise of result of the loader function if the item is a loader ', () => { | ||
const registry = new Registry(); | ||
registry.registerLoader('a', () => 'testValue'); | ||
return registry.getAsPromise('a').then((value) => { | ||
expect(value).to.equal('testValue'); | ||
}); | ||
}); | ||
it('returns a rejected promise if the item with specified key does not exist', () => { | ||
const registry = new Registry(); | ||
return registry.getAsPromise('a').then(null, (err) => { | ||
expect(err).to.equal('Item with key "a" is not registered.'); | ||
}); | ||
}); | ||
it('If the key was registered multiple times, returns a promise of the most recent item.', () => { | ||
const registry = new Registry(); | ||
registry.registerValue('a', 'testValue'); | ||
const promise1 = registry.getAsPromise('a').then((value) => { | ||
expect(value).to.equal('testValue'); | ||
}); | ||
registry.registerLoader('a', () => 'newValue'); | ||
const promise2 = registry.getAsPromise('a').then((value) => { | ||
expect(value).to.equal('newValue'); | ||
}); | ||
return Promise.all([promise1, promise2]); | ||
}); | ||
}); | ||
describe('.remove(key)', () => { | ||
it('removes the item with given key', () => { | ||
const registry = new Registry(); | ||
registry.registerValue('a', 'testValue'); | ||
registry.remove('a'); | ||
expect(registry.get('a')).to.equal(null); | ||
}); | ||
it('does not throw error if the key does not exist', () => { | ||
const registry = new Registry(); | ||
expect(() => registry.remove('a')).to.not.throw(); | ||
}); | ||
it('returns itself', () => { | ||
const registry = new Registry(); | ||
registry.registerValue('a', 'testValue'); | ||
expect(registry.remove('a')).to.equal(registry); | ||
}); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { it, describe } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import isRequired from '../../../src/utils/isRequired'; | ||
|
||
describe('isRequired(field)', () => { | ||
it('should throw error with the given field in the message', () => { | ||
expect(() => isRequired('myField')).to.throw(Error, 'myField is required.'); | ||
}); | ||
}); |
38 changes: 38 additions & 0 deletions
38
superset/assets/spec/javascripts/utils/makeSingleton_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import { describe, it } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import makeSingleton from '../../../src/utils/makeSingleton'; | ||
|
||
describe('makeSingleton()', () => { | ||
class Dog { | ||
constructor(name) { | ||
this.name = name; | ||
} | ||
sit() { | ||
this.isSitting = true; | ||
} | ||
} | ||
describe('makeSingleton(BaseClass)', () => { | ||
const getInstance = makeSingleton(Dog); | ||
|
||
it('returns a function for getting singleton instance of a given base class', () => { | ||
expect(getInstance).to.be.a('Function'); | ||
expect(getInstance()).to.be.instanceOf(Dog); | ||
}); | ||
it('returned function returns same instance across all calls', () => { | ||
expect(getInstance()).to.equal(getInstance()); | ||
}); | ||
}); | ||
describe('makeSingleton(BaseClass, ...args)', () => { | ||
const getInstance = makeSingleton(Dog, 'Doug'); | ||
|
||
it('returns a function for getting singleton instance of a given base class constructed with the given arguments', () => { | ||
expect(getInstance).to.be.a('Function'); | ||
expect(getInstance()).to.be.instanceOf(Dog); | ||
expect(getInstance().name).to.equal('Doug'); | ||
}); | ||
it('returned function returns same instance across all calls', () => { | ||
expect(getInstance()).to.equal(getInstance()); | ||
}); | ||
}); | ||
|
||
}); |
42 changes: 42 additions & 0 deletions
42
superset/assets/spec/javascripts/visualizations/models/ChartPlugin_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import { describe, it } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import ChartPlugin from '../../../../src/visualizations/core/models/ChartPlugin'; | ||
import ChartMetadata from '../../../../src/visualizations/core/models/ChartMetadata'; | ||
|
||
describe('ChartPlugin', () => { | ||
const metadata = new ChartMetadata({}); | ||
|
||
it('exists', () => { | ||
expect(ChartPlugin).to.not.equal(undefined); | ||
}); | ||
|
||
describe('new ChartPlugin()', () => { | ||
it('creates a new plugin', () => { | ||
const plugin = new ChartPlugin({ | ||
metadata, | ||
Chart() {}, | ||
}); | ||
expect(plugin).to.be.instanceof(ChartPlugin); | ||
}); | ||
it('throws an error if metadata is not specified', () => { | ||
expect(() => new ChartPlugin()).to.throw(Error); | ||
}); | ||
it('throws an error if none of Chart or loadChart is specified', () => { | ||
expect(() => new ChartPlugin({ metadata })).to.throw(Error); | ||
}); | ||
}); | ||
|
||
describe('.register(key)', () => { | ||
const plugin = new ChartPlugin({ | ||
metadata, | ||
Chart() {}, | ||
}); | ||
it('throws an error if key is not provided', () => { | ||
expect(() => plugin.register()).to.throw(Error); | ||
expect(() => plugin.configure({ key: 'abc' }).register()).to.not.throw(Error); | ||
}); | ||
it('returns itself', () => { | ||
expect(plugin.configure({ key: 'abc' }).register()).to.equal(plugin); | ||
}); | ||
}); | ||
}); |
48 changes: 48 additions & 0 deletions
48
superset/assets/spec/javascripts/visualizations/models/Plugin_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import { describe, it } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import Plugin from '../../../../src/visualizations/core/models/Plugin'; | ||
|
||
describe('Plugin', () => { | ||
it('exists', () => { | ||
expect(Plugin).to.not.equal(undefined); | ||
}); | ||
|
||
describe('new Plugin()', () => { | ||
it('creates a new plugin', () => { | ||
const plugin = new Plugin(); | ||
expect(plugin).to.be.instanceof(Plugin); | ||
}); | ||
}); | ||
|
||
describe('.configure(config, replace)', () => { | ||
it('extends the default config with given config when replace is not set or false', () => { | ||
const plugin = new Plugin(); | ||
plugin.configure({ key: 'abc', foo: 'bar' }); | ||
plugin.configure({ key: 'def' }); | ||
expect(plugin.config).to.deep.equal({ key: 'def', foo: 'bar' }); | ||
}); | ||
it('replaces the default config with given config when replace is true', () => { | ||
const plugin = new Plugin(); | ||
plugin.configure({ key: 'abc', foo: 'bar' }); | ||
plugin.configure({ key: 'def' }, true); | ||
expect(plugin.config).to.deep.equal({ key: 'def' }); | ||
}); | ||
it('returns the plugin itself', () => { | ||
const plugin = new Plugin(); | ||
expect(plugin.configure({ key: 'abc' })).to.equal(plugin); | ||
}); | ||
}); | ||
|
||
describe('.resetConfig()', () => { | ||
it('resets config back to default', () => { | ||
const plugin = new Plugin(); | ||
plugin.configure({ key: 'abc', foo: 'bar' }); | ||
plugin.resetConfig(); | ||
expect(plugin.config).to.deep.equal({}); | ||
}); | ||
it('returns the plugin itself', () => { | ||
const plugin = new Plugin(); | ||
expect(plugin.resetConfig()).to.equal(plugin); | ||
}); | ||
}); | ||
}); |
65 changes: 65 additions & 0 deletions
65
superset/assets/spec/javascripts/visualizations/models/Preset_spec.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
import { describe, it } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import Preset from '../../../../src/visualizations/core/models/Preset'; | ||
import Plugin from '../../../../src/visualizations/core/models/Plugin'; | ||
|
||
describe('Preset', () => { | ||
it('exists', () => { | ||
expect(Preset).to.not.equal(undefined); | ||
}); | ||
|
||
describe('new Preset()', () => { | ||
it('creates new preset', () => { | ||
const preset = new Preset(); | ||
expect(preset).to.be.instanceOf(Preset); | ||
}); | ||
}); | ||
|
||
describe('.register()', () => { | ||
it('register all listed presets then plugins', () => { | ||
const values = []; | ||
class Plugin1 extends Plugin { | ||
register() { | ||
values.push(1); | ||
} | ||
} | ||
class Plugin2 extends Plugin { | ||
register() { | ||
values.push(2); | ||
} | ||
} | ||
class Plugin3 extends Plugin { | ||
register() { | ||
values.push(3); | ||
} | ||
} | ||
class Plugin4 extends Plugin { | ||
register() { | ||
const { key } = this.config; | ||
values.push(key); | ||
} | ||
} | ||
|
||
const preset1 = new Preset({ | ||
plugins: [new Plugin1()], | ||
}); | ||
const preset2 = new Preset({ | ||
plugins: [new Plugin2()], | ||
}); | ||
const preset3 = new Preset({ | ||
presets: [preset1, preset2], | ||
plugins: [ | ||
new Plugin3(), | ||
new Plugin4().configure({ key: 'abc' }), | ||
], | ||
}); | ||
preset3.register(); | ||
expect(values).to.deep.equal([1, 2, 3, 'abc']); | ||
}); | ||
|
||
it('returns itself', () => { | ||
const preset = new Preset(); | ||
expect(preset.register()).to.equal(preset); | ||
}); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
export default class Registry { | ||
constructor(name = '') { | ||
this.name = name; | ||
this.items = {}; | ||
this.promises = {}; | ||
} | ||
|
||
has(key) { | ||
const item = this.items[key]; | ||
return item !== null && item !== undefined; | ||
} | ||
|
||
registerValue(key, value) { | ||
this.items[key] = { value }; | ||
delete this.promises[key]; | ||
return this; | ||
} | ||
|
||
registerLoader(key, loader) { | ||
this.items[key] = { loader }; | ||
delete this.promises[key]; | ||
return this; | ||
} | ||
|
||
get(key) { | ||
const item = this.items[key]; | ||
if (item) { | ||
return item.loader ? item.loader() : item.value; | ||
} | ||
return null; | ||
} | ||
|
||
getAsPromise(key) { | ||
const promise = this.promises[key]; | ||
if (promise) { | ||
return promise; | ||
} | ||
const item = this.get(key); | ||
if (item) { | ||
const newPromise = Promise.resolve(item); | ||
this.promises[key] = newPromise; | ||
return newPromise; | ||
} | ||
return Promise.reject(`Item with key "${key}" is not registered.`); | ||
} | ||
|
||
remove(key) { | ||
delete this.items[key]; | ||
delete this.promises[key]; | ||
return this; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export default function isRequired(field) { | ||
throw new Error(`${field} is required.`); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my only comment at this point is whether we should throw an error if a value already exists, that would require people to consciously call
.remove()
and possibly raise this to their attn if they weren't aware of it?or we could just log a warning 🤔 wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may happen a lot though for the presets. For example, registering a preset that contains all common charts then override
bar
chart with a new plugin. Some presets may reference another preset and override certain charts.I think throwing an error would be too harsh.
Can add warning but then that will mean for every plugin we have to add
.remove()
call before.register()
to avoid the warning. If every plugin does that, would a warning still be useful?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's a fair point, sgtm 👍