Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

Convert number-format to TypeScript #75

Merged
merged 5 commits into from
Jan 15, 2019
Merged

Convert number-format to TypeScript #75

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c60c8f
Convert number-format to TypeScript
kristw Jan 9, 2019
a72cad5
return string
kristw Jan 9, 2019
72a2764
unit test 100%
kristw Jan 9, 2019
cab9c1f
Allow developer to specify return type for loader
kristw Jan 10, 2019
38e9048
add doc
kristw Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions packages/superset-ui-core/src/models/Registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,30 @@ interface ItemWithValue<T> {
}

interface ItemWithLoader<T> {
loader: () => T | Promise<T>;
loader: () => T;
}

export interface RegistryConfig {
name?: string;
overwritePolicy?: OverwritePolicy;
}

export default class Registry<V> {
/**
* Registry class
*
* Can use generic to specify type of item in the registry
* @type V Type of value
* @type W Type of value returned from loader function when using registerLoader().
* W can be either V, Promise<V> or V | Promise<V>
* Set W=V when does not support asynchronous loader.
* By default W is set to V | Promise<V> to support
* both synchronous and asynchronous loaders.
*/
export default class Registry<V, W extends V | Promise<V> = V | Promise<V>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for simplicity could we just say that this has to be V | Promise<V>? it seems like we shouldn't really need to support anything beyond that since ultimately it should match V right?

Copy link
Contributor Author

@kristw kristw Jan 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the added complexity. The high-level idea is to be able to define synchronous registry. I am considering adding a subclass of Registry just to alias and hide this multiple generics.

class SyncRegistry<V> extends Registry<V, V>{}

W affect the .get() function in particular if the returned value is always the value or can also be a promise of the value. If it can be either promise of value then the subclass has to always consider if the value is promise or not. Using the W generic add these benefits:

  • Check to make sure loader is not async.
  • Knowing that the returned type of .get is not a promise simplifies the subclass operations. (Not having to manually override/cast)

name: string;
overwritePolicy: OverwritePolicy;
items: {
[key: string]: ItemWithValue<V> | ItemWithLoader<V>;
[key: string]: ItemWithValue<V> | ItemWithLoader<W>;
};

promises: {
Expand Down Expand Up @@ -70,7 +81,7 @@ export default class Registry<V> {
return this;
}

registerLoader(key: string, loader: () => V | Promise<V>) {
registerLoader(key: string, loader: () => W) {
const item = this.items[key];
const willOverwrite =
this.has(key) && (('loader' in item && item.loader !== loader) || 'value' in item);
Expand All @@ -89,7 +100,7 @@ export default class Registry<V> {
return this;
}

get(key: string): V | Promise<V> | undefined {
get(key: string): V | W | undefined {
const item = this.items[key];
if (item !== undefined) {
if ('loader' in item) {
Expand All @@ -109,7 +120,7 @@ export default class Registry<V> {
}
const item = this.get(key);
if (item !== undefined) {
const newPromise = Promise.resolve(item);
const newPromise = Promise.resolve(item) as Promise<V>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this and the function return type not be W? I guess either is okay technically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is always a promise.
The function return type has to be V|W|undefined because if W is set to Promise<V>, W won't include V.

this.promises[key] = newPromise;

return newPromise;
Expand All @@ -120,7 +131,7 @@ export default class Registry<V> {

getMap() {
return this.keys().reduce<{
[key: string]: V | Promise<V> | undefined;
[key: string]: V | W | undefined;
}>((prev, key) => {
const map = prev;
map[key] = this.get(key);
Expand Down Expand Up @@ -148,22 +159,22 @@ export default class Registry<V> {
return Object.keys(this.items);
}

values(): Array<V | Promise<V> | undefined> {
values(): (V | W | undefined)[] {
return this.keys().map(key => this.get(key));
}

valuesAsPromise(): Promise<V[]> {
return Promise.all(this.keys().map(key => this.getAsPromise(key)));
}

entries(): Array<{ key: string; value: V | Promise<V> | undefined }> {
entries(): { key: string; value: V | W | undefined }[] {
return this.keys().map(key => ({
key,
value: this.get(key),
}));
}

entriesAsPromise(): Promise<Array<{ key: string; value: V }>> {
entriesAsPromise(): Promise<{ key: string; value: V }[]> {
const keys = this.keys();

return Promise.all(keys.map(key => this.getAsPromise(key))).then(values =>
Expand Down
7 changes: 5 additions & 2 deletions packages/superset-ui-core/src/models/RegistryWithDefaultKey.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ export interface RegistryWithDefaultKeyConfig extends RegistryConfig {
setFirstItemAsDefault?: boolean;
}

export default class RegistryWithDefaultKey<V> extends Registry<V> {
export default class RegistryWithDefaultKey<
V,
W extends V | Promise<V> = V | Promise<V>
> extends Registry<V, W> {
initialDefaultKey?: string;
defaultKey?: string;
setFirstItemAsDefault: boolean;
Expand Down Expand Up @@ -41,7 +44,7 @@ export default class RegistryWithDefaultKey<V> extends Registry<V> {
return this;
}

registerLoader(key: string, loader: () => V | Promise<V>) {
registerLoader(key: string, loader: () => W) {
super.registerLoader(key, loader);
// If there is no default, set as default
if (this.setFirstItemAsDefault && !this.defaultKey) {
Expand Down
7 changes: 5 additions & 2 deletions packages/superset-ui-number-format/src/NumberFormatterRegistry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@ import createD3NumberFormatter from './factories/createD3NumberFormatter';
import NumberFormats from './NumberFormats';
import NumberFormatter from './NumberFormatter';

export default class NumberFormatterRegistry extends RegistryWithDefaultKey<NumberFormatter> {
export default class NumberFormatterRegistry extends RegistryWithDefaultKey<
NumberFormatter,
NumberFormatter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the = default specified for the generics in the Registry not allow you to just default to this? I guess you are saying it is returning the value not a promise of the value.

it seems a little burdensome for the users to always have to specify the same type 2wice 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set RegistryWithDefaultKey<NumberFormatter>
it defaults to RegistryWithDefaultKey<NumberFormatter, NumberFormatter | Promise<NumberFormatter>.

> {
constructor() {
super({
initialDefaultKey: NumberFormats.SI,
name: 'NumberFormatter',
});
}

get(formatterId?: string): NumberFormatter {
get(formatterId?: string) {
const targetFormat = `${formatterId || this.defaultKey}`.trim();

if (this.has(targetFormat)) {
Expand Down