[indexPatterns/create] track loading state independently #11990
Conversation
There was a problem hiding this comment.
@spalger I haven't done a full review of this PR but I looked at the most interesting bit, the loading_tracker module. If I understand its purpose and usage, it is to track whether the page is in a loading state, which may be determined by multiple async function calls (whether they are in progress or finished).
The loading_state module is very clever but I'm concerned that it might be too clever, unless I'm missing something. Could we "just" keep the loading count in the index pattern controller and have the async functions increment/decrement it as needed? I understand this puts more of a burden on each async caller but IMO the code becomes easier to follow. Again, its possible I'm missing something so please point it out if I am.
|
I love the intent behind this PR, but I agree with @ycombinator. When I read Can we look for a simpler way to track loading state? I think it's worth trying out @ycombinator's suggestion and seeing if that solves the problem and makes the code easier to follow. |
|
I didn't want it to be so easy to break, and I didn't want to just copy past the same logic all over the controller, but I can inline everything if you both think that the loading tracker is too weird. |
| @@ -20,6 +21,7 @@ uiModules.get('apps/management') | |||
| const refreshKibanaIndex = Private(RefreshKibanaIndex); | |||
| const intervals = indexPatterns.intervals; | |||
| let samplePromise; | |||
| let loadingCount = 0; | |||
There was a problem hiding this comment.
@ycombinator @cjcenizal Do you guys like this better?
There was a problem hiding this comment.
I do find this pattern easier to follow. Thanks for considering the suggestion.
| if (timeField) { | ||
| updateFieldListAndSetTimeField(results, timeField.name); | ||
| } else { | ||
| updateFieldList(results); | ||
| } | ||
| }).catch(() => { |
There was a problem hiding this comment.
Did you mean for this to be a finally?
spalger
force-pushed
the
index-patterns/loading-tracker
branch
from
e4dc54c
to
b9568b0
Compare
|
That's not an invalid index name though, it just doesn't exist. An invalid index name contains any of the characters |
…s/loading-tracker
spalger
force-pushed
the
index-patterns/loading-tracker
branch
from
b6c8ddb
to
19c7ef4
Compare
* [indexPatterns/create] make loading state independent of fieldsFetchError * [indexPatterns/create] return promises while loading * [indexPatterns/create] track loading count in controller (cherry picked from commit dde4e9a)
|
5.5/5.x: 9712cf5 |
* [indexPatterns/create] make loading state independent of fieldsFetchError * [indexPatterns/create] return promises while loading * [indexPatterns/create] track loading count in controller
When the
indexPatternview is in a loading state it communicates that by setting thefieldsFetchErrorto a loading message which is effective but renders the loading message like it's an error and isn't something we can use when other parts of the controller are loading (like the samples).With this PR the loading state is now tracked with a
LoadingTrackerthat wraps the async/loading methods in the controller and tracks how many of those functions are executing at a time, then surfaces this information withLoadingTracker#isLoading(), which is then exposed on the controller for use in the view.Since this new condition made the logic for which message to display in the create button more complicated, the logic was moved to a helper function that is executed within a watcher.