feature: EuiDataGrid type improvements to support React 18 #6958

tkajtoch · 2023-07-18T15:27:37Z

Summary

@types/react for React 18 introduced a few breaking changes, primarily in the automatic type inference area. Specifically, type inference for useCallback when passing to typed props is broken in TS version we're using (it should be fixed in v5.2 by this PR and possibly more), but we're not there yet and the upgrade isn't straightforward.

React 17 apps using EUI won't be affected by these changes and React 18 apps will work as before depending on the TypeScript version they're running on.

There are also a few small fixes to ensure safe typing in other EuiDataGrid places.

Please pay attention to the JSXElementConstructor changes as it's the one that may cause the most trouble. I made sure the old and new types resolve to almost the same thing, but JSXElementConstructor had to be replaced as it breaks prop type inference when used and creates strange issues throughout our codebase when using @types/react v18. Overall, I think we should standardize whether we accept react elements or components in props in order to make sure we're rendering allowed input types correctly and not just casting the values to either of these in our render functions.

QA

Checkout this branch
Run yarn
Run yarn tsc --noEmit and confirm there are no errors in src/components/datagrid

Remove or strikethrough items that do not apply to your PR.

General checklist

Added or updated jest and cypress tests

cee-chen · 2023-07-18T16:16:22Z

src/components/datagrid/data_grid_types.ts

+    | ((props: EuiDataGridCellValueElementProps) => ReactNode)
+    | ComponentClass<EuiDataGridCellValueElementProps>;
  renderCellPopover?:
    | JSXElementConstructor<EuiDataGridCellPopoverElementProps>
    | ((props: EuiDataGridCellPopoverElementProps) => ReactNode);


Just curious, why did renderCellPopover not need to be converted from JSXElementConstructor to ComponentClass?

it doesn't throw any errors in our code and I wanted to keep this already complex change as small as possible. We should discuss replacing all JSXElementConstructor usages with more compatible types so it's consistent across the board

👍 Gotcha, I can make a follow-up issue for datagrid types related to component typing at a later date.

cee-chen · 2023-07-18T16:22:31Z

src/components/datagrid/data_grid_types.ts

-  | JSXElementConstructor<EuiDataGridColumnCellActionProps>
-  | ((props: EuiDataGridColumnCellActionProps) => ReactNode);
+export type EuiDataGridColumnCellAction = (
+  props: EuiDataGridColumnCellActionProps


Cell actions also need to be able to take class components if necessary (I don't know how many consumsers are still using those, but they're valid and we should accept them type-wise).

Should we consider creating a new shared type for this? Something that takes a set of props as a generic, and then returns either a function component or class component version? We have similar type helpers in src/components/common that might be worth taking a leaf from.

I updated the type to ComponentType<EuiDataGridColumnCellActionProps> which seems to work just fine and checks all the boxes

Nice! I was experimenting with using ComponentType<> across the board in data_grid_types.ts (d9a9974) and it seemed to work great locally in VSCode - I wasn't getting any reported errors. I did get errors however when I ran yarn tsc along the lines of:

src/components/datagrid/data_grid.test.tsx:2031:11 - error TS2322: Type '({ rowIndex, columnId }: EuiDataGridCellValueElementProps) => string' is not assignable to type 'ComponentType<EuiDataGridCellValueElementProps>'. Type '({ rowIndex, columnId }: EuiDataGridCellValueElementProps) => string' is not assignable to type 'FunctionComponent<EuiDataGridCellValueElementProps>'. Type 'string' is not assignable to type 'ReactElement<any, any> | null'. 2031 renderCellValue={({ rowIndex, columnId }) =>

Not sure which is reporting more correctly, vscode or CLI 😅 Either way, we can punt investigating that til later, or that commit works locally for you, feel free to cherry-pick it - whatever works!

src/components/datagrid/controls/display_selector.tsx

cee-chen

Overall this looks great - I just have 2 change requests. 1 code cleanup in display_selectors.tsx and one bug/change request in data_grid_toolbar.tsx

cee-chen · 2023-07-18T17:59:53Z

src/components/datagrid/utils/in_memory.tsx

@@ -33,7 +33,7 @@ export const useInMemoryValues = (
  rowCount: number
 ): [
  EuiDataGridInMemoryValues,
-  (rowIndex: number, columnId: string, value: string) => void
+  EuiDataGridInMemoryRendererProps['onCellRender']


Super nice cleanup! 🎉

cee-chen · 2023-07-18T18:19:27Z

src/components/datagrid/controls/data_grid_toolbar.tsx

+  }
+
+  if (position === 'left.prepend') {
+    return additionalControlsObj.left as ReactNode;


This is missing rendering additionalControls.left.prepend and additionalControls.left.append - you can see several failing data_grid_toolbar tests.

I'm also not a super huge fan personally of the early return syntax; my 2c is that it feels way harder to read and understand what's going on (as evidenced by the failing missed cases 😅). I'm going to take a pass at cleaning this up more and send that your way.

Yeah I'm not super proud of this one, I just wanted to give it more structure so TS would be happy. I have no idea how I missed these failing tests, I ran the entire test suite and I'd swear it was green 🤔 Anyway, if you wanna try refactoring it I'd really appreciate it!

LMK what you think!

https://github.com/tkajtoch/eui/compare/feature/react-18-ts-datagrid-enhancements...cee-chen:eui:feature/react-18-ts-datagrid-enhancements?expand=1

I'm not sure it's a huge improvement, but at least there aren't torturous type ternaries 😅 If you're good with it I can push it up to this branch, or feel free to cherry-pick, up to you.

Also, I made a unrelated typeof boolean change at the very bottom of the file - VSCode was telling me it was a problem locally, although I'm not sure if typescript linting would have said the same 🤷

Your code looks so much better, thank you! I committed it here

cee-chen · 2023-07-24T19:06:24Z

stale PR check-in - @tkajtoch let me know if you have any questions about the comments I left above, or if you'd like me to push up my change requests directly to this branch to save you some time - happy to do so!

…nference issues on React 18

src/components/datagrid/controls/__snapshots__/column_sorting.test.tsx.snap

cee-chen

Thanks for the thorough work on this Tomasz! Let's merge this in.

BTW, I may swing a PR for the datagrid component types cleanup by you after this. I'm a little confused because my local VSCode vs local yarn tsc is not quite matching up 🤔

tkajtoch requested review from cee-chen and a team July 18, 2023 15:27

tkajtoch self-assigned this Jul 18, 2023

cee-chen reviewed Jul 18, 2023

View reviewed changes

src/components/datagrid/controls/display_selector.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Jul 18, 2023

View reviewed changes

tkajtoch force-pushed the feature/react-18 branch from 33861c2 to 0b15098 Compare July 24, 2023 20:26

tkajtoch added 8 commits July 24, 2023 22:47

fix: type EuiDataGridCell context

222ee71

fix: type pagination and sorting callbacks

d442619

fix: type EuiDataGrid cell and action callbacks

d5792fb

fix: add remaining EuiDataGrid types

395f0c8

refactor: modify renderAdditionalControls to ensure correct typing

32692b2

fix!: update EuiDataGridCellProps renderCellValue types to fix type i…

50a5a27

…nference issues on React 18

refactor: make renderAdditionalControls cleaner

8e28960

fix: allow declare fields

8b4ca34

tkajtoch force-pushed the feature/react-18-ts-datagrid-enhancements branch from d75b55b to 64b47f4 Compare July 24, 2023 21:08

cee-chen reviewed Jul 24, 2023

View reviewed changes

src/components/datagrid/controls/__snapshots__/column_sorting.test.tsx.snap Outdated Show resolved Hide resolved

tkajtoch force-pushed the feature/react-18-ts-datagrid-enhancements branch from 64b47f4 to 6ef48b4 Compare July 24, 2023 22:44

fix: use event.currentTarget instead of event.target

43d487e

tkajtoch force-pushed the feature/react-18-ts-datagrid-enhancements branch from 6ef48b4 to 43d487e Compare July 24, 2023 22:45

fix: update EuiDataGridColumnCellAction type to ComponentType

a112385

cee-chen approved these changes Jul 24, 2023

View reviewed changes

tkajtoch merged commit 963fac3 into elastic:feature/react-18 Jul 25, 2023

tkajtoch added a commit that referenced this pull request Jul 26, 2023

feature: EuiDataGrid type improvements to support React 18 (#6958)

dc5f3ea

tkajtoch added a commit that referenced this pull request Jul 28, 2023

feature: EuiDataGrid type improvements to support React 18 (#6958)

900986b

tkajtoch added a commit that referenced this pull request Jul 31, 2023

feature: EuiDataGrid type improvements to support React 18 (#6958)

ab7f6cc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: EuiDataGrid type improvements to support React 18 #6958

feature: EuiDataGrid type improvements to support React 18 #6958

tkajtoch commented Jul 18, 2023 •

edited by cee-chen

Loading

cee-chen Jul 18, 2023

tkajtoch Jul 18, 2023

cee-chen Jul 18, 2023 •

edited

Loading

cee-chen Jul 18, 2023 •

edited

Loading

tkajtoch Jul 24, 2023

cee-chen Jul 25, 2023

cee-chen left a comment

cee-chen Jul 18, 2023

cee-chen Jul 18, 2023 •

edited

Loading

tkajtoch Jul 18, 2023

cee-chen Jul 18, 2023 •

edited

Loading

tkajtoch Jul 24, 2023

cee-chen commented Jul 24, 2023

cee-chen left a comment •

edited

Loading

feature: EuiDataGrid type improvements to support React 18 #6958

feature: EuiDataGrid type improvements to support React 18 #6958

Conversation

tkajtoch commented Jul 18, 2023 • edited by cee-chen Loading

Summary

QA

General checklist

cee-chen Jul 18, 2023

Choose a reason for hiding this comment

tkajtoch Jul 18, 2023

Choose a reason for hiding this comment

cee-chen Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

tkajtoch Jul 24, 2023

Choose a reason for hiding this comment

cee-chen Jul 25, 2023

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen Jul 18, 2023

Choose a reason for hiding this comment

cee-chen Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

tkajtoch Jul 18, 2023

Choose a reason for hiding this comment

cee-chen Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

tkajtoch Jul 24, 2023

Choose a reason for hiding this comment

cee-chen commented Jul 24, 2023

cee-chen left a comment • edited Loading

Choose a reason for hiding this comment

tkajtoch commented Jul 18, 2023 •

edited by cee-chen

Loading

cee-chen Jul 18, 2023 •

edited

Loading

cee-chen Jul 18, 2023 •

edited

Loading

cee-chen Jul 18, 2023 •

edited

Loading

cee-chen Jul 18, 2023 •

edited

Loading

cee-chen left a comment •

edited

Loading