Add basic React 18 support #6827

tkajtoch · 2023-06-02T21:54:49Z

Summary

This is the first set of changes needed to make EUI compatible with React 18. Following @cee-chen suggestion in #6772, I divided the work into more branches that are easier to review and merge into our feature branch (feature/react-18).

This PR contains:

Upgrade react, react-dom and its types to v18
Update package.json resolutions to force @types/react version to v18
Fix component types to follow React 18 changes, most notably add PropsWithChildren to components accepting the children property
Switch to using the new createRoot API in docs entrypoint

QA

Pull the changes and run yarn to fetch new dependencies
Run yarn build and confirm the application builds successfully
Run yarn start, wait for it to load and open http://localhost:8030 to confirm the application is loading
See the homepage and other component pages opening as expected

Please note that the work is not final and there are additional changes that need to be made to make the application stable and tests passing. At this stage, some pages may crash the application, portals are not working and the majority of tests are failing. This is expected and will be addressed in next PRs.

General checklist

Checked in Chrome, Safari, Edge, and Firefox

src-docs/src/store/configure_store.js

cee-chen

Hey Tomasz, huge apologies for taking so long to get to this PR while you were away. We had a fairly large set of dependency changes during your PTO (Node 18 upgrade being the biggest one that would affect this work). Would you mind rebasing/merging in latest main again, as well as running yarn/yarn lint-fix? (I'm seeing some newline/whitespace changes that are a little suspicious / that I wouldn't expect to see on latest Prettier).

Also I hate to be annoying, but I think we may need to either split up the type changes into another PR or go through each more slowly/carefully. There's way more changes than I would have expected to types (I was hoping it would be just the children change, but alas) and I'm relatively concerned about it. For better or for worse, we get a decent amount of Typescript questions and feedback, particularly around our components with generics, e.g. tables, select components, etc. - I'm a little surprised to see some of these types that used to be inferred now needing to be explicitly stated.

i18ntokens.json

.babelrc.js

package.json

cee-chen · 2023-06-19T04:39:09Z

package.json

+    "@types/react": "^16.9 || ^17.0 || ^18.0",
+    "@types/react-dom": "^16.9 || ^17.0 || ^18.0",
    "moment": "^2.13.0",
    "prop-types": "^15.5.0",
-    "react": "^16.12 || ^17.0",
+    "react": "^16.12 || ^17.0 || ^18.0",


[not a change request, just writing this out to remind myself] Once the feature branch is ready to merge, we should make sure to test the final EUI build/package against a dummy app (CRA?) using React 16/17 to ensure it's still backwards compatible

cee-chen · 2023-06-19T04:42:34Z

src/components/basic_table/basic_table.tsx

  [id: string]: ReactNode;
 }

-export function getItemId<T>(item: T, itemId?: ItemId<T>) {


The repeated extends object type changes here feel like a red flag / code smell - worst case scenario is that this will affect the typing of every single table usage in Kibana. What happened to make this change necessary?

cee-chen · 2023-06-19T04:57:52Z

src-docs/src/index.js

+const root = createRoot(document.getElementById('guide'));
+
+root.render(
+  <StrictMode>


Just to confirm, do we deliberately want strict mode on our docs? Does React have a recommendation/default on this moving forward?

I'd say so. StrictMode runs additional checks during runtime and lets us know about potential issues with our code. Since the docs site is heavily used as a local development preview, I find it extremely handy to have it running React in strict mode.

I think the reason it's not enabled by default in React 18 is more for compatibility purposes than to let people choose whether they actually want their application to be strictly checked when running in development environments.

I think my hesitation is purely around debugging. I can definitely see us "forgetting" or not realizing this is turned on locally, and thinking we can repro a bug in dev but not prod or vice versa. Personally, I think the most helpful option for us as frontend devs would be to add a toggle to our main header that allows us to toggle strict mode on or off - this will give us a visual hint that strict mode is enabled, and allow us to disable it dynamically to repro environments for specific bugs if necessary.

I'm not sure if there can be issues with non-strict mode when writing and testing code in strict mode. StrictMode also only affects development environments and EUI has it enabled only for the docs site. The main difference is that components are mounted twice when running in development mode to detect errors:

To help surface these issues, React 18 introduces a new development-only check to Strict Mode. This new check will automatically unmount and remount every component, whenever a component mounts for the first time, restoring the previous state on the second mount.
(source)

I've never seen anyone having an option like that but maybe there's a reason for it. What do you say we leave it for now and add something like this if we actually find a non-StrictMode error we need to fix?

Gotcha. Honestly it'll be a bit of an adjustment for me, I've never really worked in strict mode before and I wouldn't have expected, for example, double console logs due to remounting.

I might play around a bit later with adding a switcher for strict mode that goes next to our babelfish switch if you don't object - but that can go into the feature branch later.

src-docs/src/index.js

src-docs/src/store/configure_store.js

src-docs/src/views/tables/custom/custom.tsx

i18ntokens.json

yarn.lock

cee-chen · 2023-06-21T03:43:25Z

src/components/breadcrumbs/breadcrumb.tsx

+} & Pick<EuiBreadcrumbProps, 'truncate'> &
+  PropsWithChildren;

 export const EuiBreadcrumb: FunctionComponent<
  HTMLAttributes<HTMLLIElement> & _EuiBreadcrumbProps


@tkajtoch We have a majority on using the non-generic syntax it looks like - can you update all instances in this PR to use the agreed-upon syntax? Or alternatively, would you be open to separating out the types update to a separate PR to make reviewing a little easier?

tkajtoch · 2023-06-22T09:50:21Z

@cee-chen I rebased the branch and removed most of the unrelated changes to keep reviewing as straightforward as possible. I have them stashed to include in separate PRs when this one is merged into our feature branch.

tkajtoch · 2023-06-26T16:53:17Z

src-docs/src/index.js

  ...childRoutes,
 ];

-ReactDOM.render(


Caution, ugly github diff below! It's way better to compare changes side to side - it just wraps the app in <StrictMode>.

Random GitHub pro tip - using the Hide whitespace setting or adding ?w=0 to the PR URL is super helpful for added wrappers like these!

tkajtoch · 2023-06-26T16:56:42Z

src/components/portal/portal.tsx

-      ? createPortal(this.props.children, this.portalNode)
-      : null;
+    return this.portalNode ? (
+      <>{createPortal(this.props.children, this.portalNode)}</>


It's wrapped in JSX fragment because createPortal returns ReactPortal which isn't a supported FunctionComponent return type

Interesting - so the change is purely for typing and not because it affects actual end behavior? 🤔

Just curious, why doesn't this affect these other instances in that case?

eui/src/components/datagrid/utils/in_memory.tsx

Line 179 in 976e603

return createPortal(

eui/src/components/page_template/bottom_bar/page_bottom_bar.tsx

Lines 71 to 73 in d549ff0

return hasValidAnchor && parentNode.current

? createPortal(bar, parentNode.current)

: bar;

Or do you mean that class components no longer support this return type (since this component is a class, not function component)?

Yeah, sorry, I meant class components and render() return value!

Gotcha!! Thanks for clarifying!

package.json

cee-chen · 2023-06-27T01:02:58Z

src/components/code/code_block_annotations.tsx

+type EuiCodeBlockAnnotationProps = PropsWithChildren &
+  CommonProps & {


Extra optional thought - if we're already exporting our own CommonProps type, would it make sense to just change it to CommonPropsWithChildren that extends PropsWithChildren? That way we only need one extra import? 🤷

Probably not a huge deal in the long run, just throwing that thought out there

@tkajtoch just a quick re-ping on this question - just wanted to get your thoughts on it! Feel free to tell me it's an unnecessary abstraction 😅

Sorry, looks like I missed this one!

CommonProps is used in components that both accept and don't accept passing children, so we'd need to have CommonProps as it is right now and define the new type CommonPropsWithChildren = CommonProps & PropsWithChildren.

I like that it would give us one less type to extend props with. On the other hand we'd have to document to not use the default PropsWithChildren and instead use our custom type and I don't know how I feel about that. Maybe it's just better to keep things as they are. I'm not sure. We can always switch if we feel there's a value in it - let's discuss this with the team during our next meeting!

No worries at all! Yeah it'd be a super minor syntactical sugar, just wanted to get your quick thoughts on whether you thought it was worth it. If you don't think it is, that's totally cool!

src/components/datagrid/body/data_grid_body_virtualized.tsx

src/components/form/form.styles.test.tsx

cee-chen · 2023-06-27T01:20:47Z

src/components/image/image_types.ts

+> &
+  PropsWithChildren & {


Also another annoying question that you are totally free to say you'd prefer not to enforce, but I wonder if we want to be more consistent with the order in which we declare PropsWithChildren? Would it be easier to tell if a component has children at a glance if we declared it (and CommonProps) first? e.g.,

// Least to most specific type EuiComponentProps = PropsWithChildren & CommonProps & ExtendedComponentProps & { // ... props specific to this component }

I love this idea! I updated the lines I changed to reflect that but I'd love us to document it actually make it a rule, just like module imports order for example.

cee-chen · 2023-06-27T01:47:13Z

Those are all my remaining comments - a lot of them are more general questions rather than change requests. This was a breeze to review, thanks for pruning it down Tomasz! ❤️‍🔥

…in babel config

…dren

…ecific

cee-chen

Thanks for taking the time to answer my q's / address my comments, it's super appreciated!! I think I have one outstanding question I just wanted to get your thoughts on, but it's not a change request - feel free to merge in! 🚢 🎉

* build(deps): Bump react and react-dom to v18 * refactor(docs): update docs site initialization to use createRoot API * build(babel): enforce classic react runtime and allow declare fields in babel config * refactor(*): update children prop types to use React 18 PropsWithChildren * build(*): bump react and react-dom versions * style(EuiForm): replace React.FC with FunctionComponent usage * style(*): reorder PropsWithChildren usage to be from least to most specific

tkajtoch added skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) tech debt labels Jun 2, 2023

tkajtoch requested a review from a team June 2, 2023 21:54

tkajtoch self-assigned this Jun 2, 2023

tkajtoch mentioned this pull request Jun 2, 2023

React 18 upgrade #6772

Closed

14 tasks

tkajtoch commented Jun 2, 2023

View reviewed changes

src-docs/src/store/configure_store.js Show resolved Hide resolved

cee-chen reviewed Jun 19, 2023

View reviewed changes

cee-chen suggested changes Jun 21, 2023

View reviewed changes

tkajtoch force-pushed the feat/react-18-upgrade-packages-and-types branch 2 times, most recently from e9209a6 to d0de9db Compare June 22, 2023 09:33

tkajtoch requested a review from cee-chen June 22, 2023 09:56

tkajtoch mentioned this pull request Jun 22, 2023

feat(DragAndDrop): Switch from react-beautiful-dnd to its fork @hello-pangea/dnd #6868

Merged

7 tasks

tkajtoch force-pushed the feat/react-18-upgrade-packages-and-types branch 2 times, most recently from 3ff72f4 to b7d23a5 Compare June 26, 2023 16:49

tkajtoch commented Jun 26, 2023

View reviewed changes