Explore: Change Logs.tsx to a functional component #87808
Conversation
…na/logs-functional
onHiddenSeriesChanged should be called only when dataWithConfig changes, not when the callback changes itself. This is actually causing an infinite loop because onHiddenSeriesChanged may not be memoized in the parent and passed as a new callback function on each render.
…na/logs-functional
gelicia
force-pushed
the
kristina/logs-functional
branch
from
743297a
to
867f480
Compare
gelicia
added
no-backport
There was a problem hiding this comment.
Thanks a lot for taking care of this refactor!
In general this is a very big change that doesn't have enough previous testing coverage to guide it. In general what worries me the most is the problem with dependency arrays. Disabling it will cause the effects and clean up functions to use potentially stale references with unexpected results, so it gives me very little confidence to release it as it is. I would greatly appreciate if you can work around them.
If I can help with this, I'm happy to do it, as this is a key component for logs in Explore.
| const toggleLegendRef: React.MutableRefObject<(name: string, mode: SeriesVisibilityChangeMode) => void> = | ||
| React.createRef(); | ||
| const topLogsRef = React.createRef<HTMLDivElement>(); |
There was a problem hiding this comment.
I think these could be useRef().
| ); | ||
| } | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Disabling exhaustive deps should be an exceptional measure of last resort, so in general in this PR I would ask you to not use it.
For this particular case, the clean up function will be created and returned with values that might not be valid or present when it's executed, specially those pointing to timers.
There was a problem hiding this comment.
Good catch! We may need to use useRef to store latest panelState.logs and timers and call it only on unmount. In long-run this code looks like should not be here at all and Explore should clean up panel state for each panel automatically 🤔
| }) | ||
| ); | ||
| } | ||
| if (this.props.panelState?.logs?.visualisationType !== prevProps.panelState?.logs?.visualisationType) { | ||
| const visualisationType = this.props.panelState?.logs?.visualisationType ?? getDefaultVisualisationType(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Same here. Forcing the dependencies to not update should not be the solution, because the point of the dependency array and the linting rule is to use valid referenced values and prevent bugs.
| behavior: 'auto', | ||
| top: 0, | ||
| }); | ||
| } | ||
| } | ||
| this.topLogsRef.current?.scrollIntoView(); | ||
| topLogsRef.current?.scrollIntoView(); |
There was a problem hiding this comment.
Aprox from line 314 to 634, these used to be class methods that would not be recreated on every render. Could you please wrap them around useCallback(). The reason is to maintain the previous behavior and to not cause performance regressions and/or unnecessary re-renders.
register(...) changes outlineItems. A function that calls register(...) behaves like a setter for the state - it cannot react to the state itself.
|
@matyax / @grafana/observability-logs -> it's ready for another round of review! |
What is this feature?
This changes Logs.tsx to be a functional component. There should be no visible difference to the end user.
Why do we need this feature?
Converting this component to be functional will help further improvements. See #86431 (comment) for more details.
Which issue(s) does this PR fix?:
Fixes #87047
Special notes for your reviewer:
Please check that: