chore: Convert client metrics controller to typescript #831
Conversation
checketts
force-pushed
the
convert-metrics-controller-to-typescript
branch
from
2fc75c6
to
01f0977
Compare
|
Thanks, this looks decent. I tried to rebase it against master, but was unable to push back to the PR. Would you mind rebasing it when you have time? |
|
I will take rebase of this tomorrow. |
checketts
force-pushed
the
convert-metrics-controller-to-typescript
branch
from
01f0977
to
c1b4a9b
Compare
|
Sorry for the delay. I did a rebase against master and pushed it up. There were quite a few conflicts with index.ts, so I'll look into that some more. |
checketts
force-pushed
the
convert-metrics-controller-to-typescript
branch
from
c1b4a9b
to
facf85b
Compare
|
Looks like the tests were migrated to jest (Yay!) So that caused some breakage. I'll try to get back to them later today. |
|
Thanks for updating. Sorry about the jest migration, we just felt the need to get it done to have some more power for our tests 🔥 |
I took the liberty to convert the test to jest. |
|
Sorry I haven't been able to tackle this. Thanks for helping get it in a good state. |
| private eventStore: EventStore, | ||
| private getLogger: LogProvider, | ||
| private bulkInterval = FIVE_SECONDS, | ||
| private announcementInterval = FIVE_MINUTES, |
There was a problem hiding this comment.
I would have prefered the constructor to be in line with the format utilized by all the other services converted to typescript, something in the line of:
---
constructor(
stores: Pick<IUnleashStores,
| 'clientMetricsStore'
| 'strategyStore'
| 'featureToggleStore'
| 'clientApplicationsStore'
| 'clientInstanceStore'
| 'eventStore'
>,
config: Pick<IUnleashConfig, 'getLogger'>,
) {
this.clientMetricsStore = stores.clientMetricsStore;
this.strategyStore = stores.strategyStore;
this.toggleStore = stores.featureToggleStore;
this.clientAppStore = stores.clientApplicationsStore;
this.clientInstanceStore = stores.clientInstanceStore;
this.eventStore = stores.eventStore;
----
There was a problem hiding this comment.
Your proposal mirrors the JS approach, but I don't think it makes the class easier to use, especially since the stores variable is immediately unwrapped into the individual stores.
My change just moved the unwrapping to outside the constructor.
I'm fine if you want it changed back for consistency, but I think simplifying the classes for clarity and testing is worth doing.
There was a problem hiding this comment.
With the Pick format it will actually be the same from the test perspective. You will only have to provide the requirements from the Picks and you will get the type-support from TypeScript.
There was a problem hiding this comment.
Interesting! So in the tests I won't have to create an IUnleashStores with all keys before the pick? Or would you use an any type on that?
Learning more here: https://www.typescriptlang.org/docs/handbook/utility-types.html#picktype-keys
Since you are in the code, are you going to make that change?
There was a problem hiding this comment.
Since you are in the code, are you going to make that change?
On it!
There was a problem hiding this comment.
You can see the result here, basically the test provides the same as before, just wrappted in objects.
src/lib/services/index.ts
Outdated
| stores.clientInstanceStore, | ||
| stores.eventStore, | ||
| config.getLogger, | ||
| ); |
There was a problem hiding this comment.
To my previous comment, this would the be new ClientMetricsService(stores, config)
No worries. Had some extra time to finally get this one moving. |
| variants: any[]; | ||
| createdAt: Date; | ||
| lastSeenAt: Date; | ||
| } |
* fix: toast in mobile view * fix: feature settings mobile view * fix: feedback in mobile * fix: add space for ts expect error * fix: change breakpoint to xs instead of sm
This is intended to just convert the metrics controller/service/store to typescript.