[DataViews] Fix checking remote clusters in empty state#110054
[DataViews] Fix checking remote clusters in empty state#110054Dosant merged 3 commits intoelastic:masterfrom
Conversation
Dosant
changed the title
|
|
||
| // loading list of index patterns | ||
| useEffect(() => { | ||
| isMounted.current = true; |
There was a problem hiding this comment.
(not related to the main fix, but way too minor to extract to separate pr)
I noticed that how this isMounted was setup is potentially risky because it was set up inside an effect with particular dependencies and then reused in other places which are likely dependent on other things.
Instead of fixing it, I removed it. (no need for it because no race condition or potential memory leaks here) see: https://github.com/facebook/react/pull/22114
| ); | ||
|
|
||
| useEffect(() => { | ||
| reloadMatchedIndices(title); |
There was a problem hiding this comment.
@sebelga pointed out that this might be redundant: #109500 (comment)
I tried to remove and indeed don't see any regressions
(not related to the main fix, but way too minor to extract to separate pr)
There was a problem hiding this comment.
👍 I'd expect the number of requests to go from 3 to 2 (without the memoizeOnce you added).
| useCallback(() => { | ||
| let isMounted = true; | ||
| if (!hasDataIndices) | ||
| useEffect(() => { |
There was a problem hiding this comment.
This is the main fix [DataViews] Fix checking remote clusters in empty state
We had useCallback instead of useEffect by mistake
Dosant
added
Feature:Data Views
|
Pinging @elastic/kibana-app-services (Team:AppServices) |
Dosant
requested review from
mattkime and
sebelga
and removed request for
mattkime
mattkime
left a comment
mattkime
left a comment
There was a problem hiding this comment.
Code looks good, works well, and thanks for the detailed notes on the changes
5 participants
Summary
Fix #108930
Also some other minor improvements