-
Notifications
You must be signed in to change notification settings - Fork 26
move monitoring relation functionality to monitoring store #2685
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
base: master
Are you sure you want to change the base?
Conversation
| isMonitoringDisabled, | ||
| isMonitoringEnabled, |
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.
Do we really need two?
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 guess it makes it more readable in the usage sites if it's a positive or negative based usage
| fetchTypedFromMonitoring, | ||
| deleteFromMonitoring, | ||
| optionsFromMonitoring, |
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.
Should these be typed?
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 don't understand what you're referring to
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.
Example, instead of await monitoringStore.fetchTypedFromMonitoring<EndpointDetails>(${monitored-endpoints}/${endpointName}?history=${historyPeriod}); we would call monitoringStore.getHistory(endpointName, historyPeriod)
Or instead of monitoringStore.fetchTypedFromMonitoring<number>("monitored-endpoints/disconnected"), we would call monitoringStore.disconnected()
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.
if we applied the same logic to the servicecontrolstore, we'd have ~100 different functions exposed from the store.
It's an idea worth exploring, but not as part of this refactor IMO
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.
not really a store, just utilities
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.
yeah but couldn't find a good place for it so I just put it coexistent with the stores that use it
| export function getParams() { | ||
| const params: Param[] = []; | ||
|
|
||
| if (!window.location.search) return params; | ||
|
|
||
| const searchParams = window.location.search.split("&"); | ||
|
|
||
| searchParams.forEach((p) => { | ||
| p = p.startsWith("?") ? p.substring(1, p.length) : p; | ||
| const singleParam = p.split("="); | ||
| params.push({ name: singleParam[0], value: singleParam[1] }); | ||
| }); | ||
| return params; | ||
| } |
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.
we probbly should use https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams#parsing_window.location
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.
can change in another PR.
part of #1905