Convert number-format to TypeScript #75
Conversation
Codecov Report
@@ Coverage Diff @@
## master #75 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 67 67
Lines 744 754 +10
Branches 130 146 +16
=====================================
+ Hits 744 754 +10
Continue to review full report at Codecov.
|
kristw
added
#code-quality
#enhancement
There was a problem hiding this comment.
looks good overall, left some comments about the added W type. If you and @xtinec think it'll enable us to be more specific I'm totally on board, but curious if the added complexity is needed vs just V | Promise<V>. Seems potentially useful in some cases?
| * 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>> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
loaderis not async. - Knowing that the returned type of
.getis not a promise simplifies the subclass operations. (Not having to manually override/cast)
| @@ -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>; | |||
There was a problem hiding this comment.
should this and the function return type not be W? I guess either is okay technically.
There was a problem hiding this comment.
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.
| export default class NumberFormatterRegistry extends RegistryWithDefaultKey<NumberFormatter> { | ||
| export default class NumberFormatterRegistry extends RegistryWithDefaultKey< | ||
| NumberFormatter, | ||
| NumberFormatter |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
If you set RegistryWithDefaultKey<NumberFormatter>
it defaults to RegistryWithDefaultKey<NumberFormatter, NumberFormatter | Promise<NumberFormatter>.
🏠 Internal
@superset-ui/number-formatto TypeScript🏆 Enhancements
string. This is different from previous behavior.nullnull'null'undefinedundefined'undefined'NaNNaN'NaN'Registry, changing fromRegistry<V> to Registry<V, W>Vis type of value in the registryWis type of value returned fromloaderfunction when using.registerLoader(key, loader).Wcan be eitherV,Promise<V>orV | Promise<V>W=Vwhen does not support asynchronous loader. Making return type of.get()becomeVinstead ofPromise<V>Wis set toV | Promise<V>to support both synchronous and asynchronous loaders.