eui
eui copied to clipboard
elastic
[EuiDataGrid] renderCellContext proposal
Summary
Currently, it's very difficult in Kibana for a plugin that provides a common usage of EuiDataGrid (ex: triggers_actions_ui) to allow plugins that consume the dependent plugin (ex: security solution) a way to define the renderCellValue prop of EuiDataGrid with all of their custom props and solution specific logic, while allowing the providing plugin a way to also inject props/change the jsx returned. This was at first solved with the first usages of EuiDataGrid across kibana with a whole lot of prop drilling (ex: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/timelines/components/timeline/cell_rendering/default_cell_renderer.tsx#L23 and it's usages) and then in the latest iteration, by having the consuming plugin 'register' functions when the plugin starts to use as hooks by the service providing plugin at runtime. This approach does not require drilling props, but is imo worse because it's very difficult (I have not been able to figure it out, and have tried a whole bunch of different things) to pass application specific props from both the consuming and service providing plugins, without either violating the rules of hooks and causing react to throw runtime errors, or by causing every cell in every table to re-render whenever any parent component does, because the renderCellValue function is referentially new each time. https://github.com/elastic/kibana/blob/main/x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table.tsx#L371-#L429 This is maybe not a big deal in the parts of kibana where there's only ever a single table, or very little complexity is needed in cells, but in security solution, where users will often manipulate the timeline table in a modal with the alerts table open underneath, or where we have cells with lots of permissions, row cell actions that fire network requests, etc etc, this is absolutely brutal for overall performance and a massive problem.
This branch proposes a new solution to this problem by introducing a new optional prop, renderCellContext, with a type of:
type RenderCellContext<T extends Record<string, any>> = () => T;
that allows a service providing plugin to pass in extra props (with the known type T) to EuiDataGrid, and then the renderCellValue prop can be a normal, memoized component, that has known type safe props. The service providing plugin can take this another step further, and allow the consuming plugin to pass their own definition of these props, combine the context values, conditionally return a different renderCellValue when the service providing plugin encounters an error/upgrade of frontend assets is needed/whatever, all in a type safe (working on improving the types, partially why this is a draft pr) way with components that can make full use of other hooks/app specific context/the performance trinity of useMemo/useCallback/memo, anything.
Below is a gif of using this branch of eui with a local build of kibana, where I've made use of these changes. The recording is of the timeline while moving from tab to tab with the react dev tools highlighting when components update. As you can see with 100 rows * 10 visible columns, none of the cells are re-rendering when they do not have to. The profiler is also doing the same, and you can see that none of the cells re-rendered at all.
Below is a gif of the same on main, and you can see the 100 rows underneath the timeline flash as the tabs change multiple times. The profiler also shows every cell in the table rendering on each tab switch, with 100 rows this amounts to hundreds of milliseconds not blocking rendering on a powerful desktop, might be seconds worth on a less powerful laptop.
Modeled this sort of on TanStack table's getContext https://github.com/TanStack/table/blob/main/packages/table-core/src/core/cell.ts#L67 however this is a lot more flexible I think.
I have the changes needed to make use of this in a branch, https://github.com/kqualters-elastic/kibana/tree/alerts-table-rerenders , there is no point in opening a draft pr because CI will fail right away, but can be tested by building the eui branch of this pr, https://github.com/kqualters-elastic/eui/tree/row-cell-context-proposal using directions in https://github.com/elastic/eui/blob/main/wiki/contributing-to-eui/testing/testing-in-kibana.md#in-kibana
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
- Browser QA
- [ ] Checked in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- Docs site QA
- [ ] Added documentation
- [ ] Props have proper autodocs (using
@defaultif default values are missing) and playground toggles - [ ] Checked Code Sandbox works for any docs examples
- Code quality checklist
- Release checklist
- [ ] A changelog entry exists and is marked appropriately.
- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
- Designer checklist
- [ ] Updated the Figma library counterpart
The test failure is only happening on react 16, seems sort of odd. Snapshot tests are imo of limited use, so could delete that one, or if there's something specific to test for I can update, just let me know.
After having had a chance to take a skim at this PR the larger size of it is giving me a bit more pause than I originally anticipated. @kqualters-elastic I may end up breaking out some of the more bite-sized changes into separate PRs after all (e.g. skeleton text) just for ease of review.
The other question I have for y'all is how soon you want this in Kibana / if you need it in before 8.12 FF - we unfortunately have another large set of EuiDataGrid changes in flight, that need to be in for 8.12, and will 100% cause merge conflicts with this one (cell actions redesign, will have DOM changes).
@cee-chen re: the skeleton text issue, that's imo more of a separate bug, can merge whenever. Although I do think I should mention that this problem is pretty widespread in eui. https://github.com/elastic/eui/blob/main/src/components/basic_table/default_item_action.tsx#L73 is another example, there are 10s-100s of them, and any place where components like that (not memoized, reinitializing variables in the middle of a render in a function component) where parent components are updating very frequently, could see pretty bad performance, as all of that jsx in the component is created new and destroyed each render. Does not matter to me when the datagrid specific changes get in, but the performance as of today is pretty bad, and so sooner would be better than later.
Not having a specific deadline for this work is a huge relief, thanks Kevin! (we have some unfortunate holiday timing and team-wide travel coming up). I can definitely prioritize this PR after the 8.12 FF work gets done.
Just curious, do you know of any kind of automated linter or scanning/perf tool we can use to catch those problematic instances? If so, I'd propose installing that tool on EUI to try and update them all at once rather than just for datagrid etc.
Just curious, do you know of any kind of automated linter or scanning/perf tool we can use to catch those problematic instances?
@cee-chen I think this eslint rule might detect a good amount of them https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md
https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-constructed-context-values.md might not be a bad idea either
What that lint rule won't catch is stuff like this: https://github.com/elastic/eui/blob/main/src/components/text/text.tsx#L49 and this https://github.com/elastic/eui/blob/main/src/components/text/text_align.tsx#L39 which is creating a new array on each render of css styles to inject into the app. The end result is something like this:
and is really just decimating the performance of kibana at the moment
oof, I remember being mildly worried about Emotion's performance in that regard but thought I was overthinking it at the time 😬
We can likely write our own custom lint rule for Emotion styles (or make it a general best practice to memoize the arrays). If you don't mind testing - can you quickly confirm that wrapping a useMemo around the cssStyles array does improve/affect the results in the screenshot you posted?
:green_heart: Build Succeeded
- Buildkite Build
- Commit: 4cab7dc5c47ca7e5df058360b44c52347e9cd1bb
History
- :green_heart: Build #1449 succeeded ad95e8587a4347a3e9a0e3c7c96205b1ee206d4c
- :green_heart: Build #1445 succeeded e8ce9c2b286ce02bea702544629efdea4f774030
- :green_heart: Build #1439 succeeded beda44c58f0ec7fd96fddf18e8125d70c479b9be
- :broken_heart: Build #1438 failed 341595a4c4af37145e47c194cb708aa2133844e7
cc @cee-chen
Preview staging links for this PR:
- Docs site: https://eui.elastic.co/pr_7374/
- Storybook: https://eui.elastic.co/pr_7374/storybook
