[ML] Fixing wizard charts repeated loading #44409
Conversation
💚 Build Succeeded |
💚 Build Succeeded |
jgowdyelastic
requested review from
walterra,
peteharverson and
alvarezmelissa87
jgowdyelastic
added
:ml
bug
|
Pinging @elastic/ml-ui |
jgowdyelastic
changed the title
jgowdyelastic
added
non-issue
💚 Build Succeeded |
| @@ -50,3 +54,43 @@ export const seriesStyle = { | |||
| visible: false, | |||
| }, | |||
| }; | |||
|
|
|||
| export function useChartSettings() { | |||
There was a problem hiding this comment.
I'm trying to wrap my head around the usage of this 😄 so I have this question: Why does this need to be a hook if you're returning the value directly from getChartSettings()? I'm not sure but could this have unexpected side effects? Just some food for thought:
- You could rename
getChartSettings()toupdateChartSettings()and removereturn cs;. This would mean changing all code that consumes the hook to react tochartSettingsonly and not rely on a value being returned immediately. - If we leave the code as is it might be worth adding a comment why this combination of it being a hook and returning a value immediately is necessary. Maybe rename
getChartSettings()toupdateAndGetChartSettings(), with justgetit might be unexpected that this function has side effects. - And yet maybe another way to approach this:
getChartSettings()could become auseEffect()itself so it would update on demand ifjobCreatororchartIntervalchanges. Then you'd always get the most up to datechartSettingsfrom the hook.
There was a problem hiding this comment.
I'm not surprised you questioned this. This ended up being the most fiddly and time consuming part of this refactor.
Originally had getChartSettings be a basic util function that took jobCreator and chartInterval as parameters. but i then decided that it could be a hook which could take advantage of the context, as per your last point. the problem here is that i then needed another useEffect in every file using this hook to trigger the loadCharts on a chartSettings change. and each of those files already have a lot of useEffects.
I think the solution is to go back to a normal, non-hook, util function that calculates and returns the chartSettings. Each file calling it will then need to have their own useState to wrap this so it can be passed into the ChartGrid component at the bottom of the file.
The fact that each file will need their own useState is the main reason i went with a custom hook, so they could share it. But I also need the calculated chartSettings immediately, hence why i was also returning a function that could supply it.
There was a problem hiding this comment.
Removed custom hook in 39d24ef
jgowdyelastic
force-pushed
the
caching-wizard-chart-data
branch
from
e7683aa
to
08f4262
Compare
💚 Build Succeeded |
💚 Build Succeeded |
* [ML] Caching wizard chart data * using shared chart settings hook * fixing settings hook and switching to memoize-one * removing custom hook
* [ML] Caching wizard chart data * using shared chart settings hook * fixing settings hook and switching to memoize-one * removing custom hook
Fixes repeated chart loading when navigating through the wizards.
All chart loading functions are wrapped in
memoize-oneto cache the last results based on the search parameters. This reduces the amount of data loaded when changing steps. especially when you get to the summary step where all the chart data would be loaded again.The
metric_selectioncomponent for each wizard has been split into two. A "live" version, as in, one which updates as the user adds detectors. and a results versionmetric_selection_summary, which is embedded in the summary step and only used to display results.Because of this split, there is now some duplicate code which can be removed in a followup PR.
The
*_summaryfiles are mainly a cut down version of the originalmetric_selectionfiles.The original
metric_selectionfiles have the results code removed.The chart settings are now in a shared custom hook, used by all wizards.
Also now correctly displays chart loading errors in a toast.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately