[EuiDataGrid] Add new cellContext prop/API
#7374
Conversation
kqualters-elastic
changed the title
|
Would also solve #5811 |
| @@ -115,7 +130,7 @@ const EuiDataGridCellContent: FunctionComponent< | |||
| rowIndex={rowIndex} | |||
| colIndex={colIndex} | |||
| schema={column?.schema || rest.columnType} | |||
| {...rest} | |||
| {...mergedProps} | |||
There was a problem hiding this comment.
Tell me if I am wrong but I think the spreading is not going to help us here.
There was a problem hiding this comment.
not sure what you mean. If any of the props change, this will all re-render, but consumers can short circuit everything via memo if everything in ...renderCellContext() is referentially equal.
There was a problem hiding this comment.
or another benefit, which might be pretty impactful for us, is if renderCellContext has only 1 prop that changes, and is used by only 1 of our RenderCellValue components, the others do not render. I think that will be especially impactful for our normal cells, not updating when inputs query params in the url change (and so refresh() needs to change), this would result in only 1/12th the cells re-rendering
| <EuiCheckbox | ||
| id={`select-row-${rowIndex}`} | ||
| aria-label="Select row" | ||
| onChange={() => {}} |
There was a problem hiding this comment.
Does the anonymous function get re-created on re-renders? Technically this array should be stable since it's outside the render, but not sure if it's worth passing const noOp = () => {} to this and the headerCellRender below?
| renderCellValue={({ rowIndex, columnId }) => | ||
| raw_data[rowIndex][columnId] | ||
| } | ||
| renderCellValue={renderCellValue} |
|
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. |
kqualters-elastic
force-pushed
the
row-cell-context-proposal
branch
from
7b79ab8
to
d15c935
Compare
|
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. |
@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: |
|
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 |
…tic/eui into row-cell-context-proposal
…from fn type to object/record + simplify `mergedProps` to spread operators + only pass context to `<CellElement />`, not to `<PopoverElement />`
cee-chen
force-pushed
the
row-cell-context-proposal
branch
from
3fd6bc6
to
ccc4e71
Compare
remove `|` and bake `Record<string, any>` into the props
cee-chen
force-pushed
the
row-cell-context-proposal
branch
from
ccc4e71
to
ad95e85
Compare
cee-chen
changed the title
cellContext prop/API
There was a problem hiding this comment.
Thanks for your patience with us while talking through this work Kevin!
As a note for other async readers, the performance changes in this PR have been moved to #7556, and this PR now only concerns itself with adding an API that makes it easy for consumers to pass data to static renderCellValue function components.
💚 Build Succeeded
History
cc @cee-chen |
|
Preview staging links for this PR:
|
`v93.2.0`⏩`v93.3.0` --- ## [`v93.3.0`](https://github.com/elastic/eui/releases/v93.3.0) - Added new `EuiDataGrid` new prop: `cellContext`, an optional object of additional props passed to the cell render function. ([#7374](elastic/eui#7374)) - `EuiBreadcrumbs`'s `popoverContent` API now accepts a render function that will be passed a `closePopover` callback, allowing consumers to close the breadcrumb popover from their popover content ([#7555](elastic/eui#7555)) **Bug fixes** - Fixed missing animation on native `EuiProgress` bar update ([#7538](elastic/eui#7538)) - Fixed an `EuiDataGrid` bug with `gridStyle.rowClasses`, where custom consumer classes that began with `euiDataGridRow` would not be correctly removed/reapplied ([#7549](elastic/eui#7549)) - Fixed a visual `EuiDataGrid` bug where `EuiCheckbox`es within control columns were not vertically centered within single height rows ([#7549](elastic/eui#7549))
…180016) ## Summary This pr makes use of a new prop (and some generic memoization fixes) in 2 eui prs merged recently (elastic/eui#7556 and elastic/eui#7374) to improve the performance of the alerts table. Mainly, the cellContext prop is now available to consumers of the triggersActionsUi alerts table to pass in custom props in a way that allows the renderCellValue/rowCellRender functions of the various EuiDataGrid prop apis to remain referentially stable across re-renders. There are also some various changes to various hooks and props found throughout plugins that use the table to improve performance. There should be no change in functionality, just a moderate to substantial reduction in time the app spends rendering the alerts table in various scenarios. Below will be some react dev tools performance profiles, main compared to this branch, with the exact same set of generated data. Main, switching from 10-100 rows:  This branch 10-100 rows:  Pretty modest gain here, 1 render is slower than any others on main, but overall less time spent rendering by about a second. Main, opening the cell popover on 2 cells  This branch, opening cellpopover  Again nothing crazy here, modest improvement. Main opening timeline and hitting refresh  This branch, opening timeline and hitting refresh  This is the case that brought this about in the first place, as security solution shares a lot of code between tables, the alerts table recreating all cells on every render was destroying performance of the timeline rendering in a flyout/modal while users were on alerts page or the rule detail page, which is probably the most common use case. 93ms in this branch vs 2500+ ms in main. This type of performance hit happens when almost any action is taken in timeline. Main, selecting 3 alerts 1 at a time  This branch selecting 3 alerts 1 at a time  Pretty substantial improvement here as well, ~2000ms vs 67ms. Apologies if some of the gifs are cut off, please try the branch vs main on your own! This was done on a local kibana in dev mode, so things like emotion are eating up more cpu than they would for a user, but there still should be improvement, and I can't think of any circumstance where things will be worse. Also this branch was a little longer lived than I would have liked, so if you are reviewing and changed something around these files recently please double check I didn't miss anything. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
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:
that allows a service providing plugin to pass in extra props 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 as needed, and go from there.
QA
General checklist
@defaultif default values are missing) and playground toggles- [ ] Checked Code Sandbox works for any docs examplesand cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)