[react@18] More breaking type fixes (should be the last pr) by Dosant · Pull Request #192266 · elastic/kibana

Dosant · 2024-09-06T13:04:29Z

Summary

in @types/react@18 types DefinitelyTyped/DefinitelyTyped#56210. This PR addresses a bunch of remaining fixes (hopefully the last mass ping PR like this) The most common are:

1 Objects are no longer considered a valid ReactNode

In types@17 the ReactNode typing was too soft, it allowed objects and functions being passed as ReactNode, e.g.

let obj: React.ReactNode = {};
let func: React.ReactNode = () => {};

This was by mistake, and this PR mutes most of such cases by simply casting to a string or ReactNode.
In some cases, it is worth to follow up and address the raised issues in a better way (see in comments)

function MyComponent() {

const error: string | Error = 'Error'

return (
  <div>
-   {error}
+   {error as string}
  </div>
)

}

Most common problems are related to rendering errors, where it could be string | Error object rendered directly as a ReactNode. Most often it is related to alerting framework:

export interface RuleFormParamsErrors {
  [key: string]: string | string[] | RuleFormParamsErrors;
}

Not sure if there is a better fix then casting, surely not short-term.

2 More `useCallback` implicit any fixes

Follow up to #191659

3 `EuiSelect` doesn't have a placeholder prop

In a couple of places, the placeholder prop was removed. This is because react types were updated and placeholder was removed from the base HTML element, so it highlighted places where placeholder prop was redundant

Dosant · 2024-09-06T13:19:44Z

packages/kbn-discover-utils/src/data_types/logs/components/log_level_badge.tsx

        css={badgeCss}
      >
-        {logLevel}
+        {logLevel as React.ReactNode}


logLevel is {} for some reason. Might be worth fixing on a level higher, but I didn't want to look into what would fail :)

Dosant · 2024-09-06T13:20:11Z

packages/shared-ux/button_toolbar/src/popover/popover.tsx

 */
 export type Props = AllowedButtonProps &
-  AllowedPopoverProps & {
+  Omit<AllowedPopoverProps, 'children'> & {


We omit children from this type to properly replace using our custom type

Dosant · 2024-09-06T13:21:08Z

packages/shared-ux/chrome/navigation/src/ui/navigation.stories.tsx

 }

-const NavigationWrapper: FC<Props & Partial<EuiCollapsibleNavBetaProps>> = (props) => {
+const NavigationWrapper: FC<Props & Omit<Partial<EuiCollapsibleNavBetaProps>, 'children'>> = (


We omit children from this type to properly replace using our custom type

Dosant · 2024-09-06T13:24:38Z

x-pack/plugins/data_visualizer/public/application/common/components/link_card/link_card.tsx

            ) : (
-              icon
+              // TODO: check this, icon might be a ComponentType
+              (icon as unknown as React.ReactNode)


The type allows ComponentType and this would render incorrectly in runtime. Worth following up (or feel free to improve in this PR)

Thanks for the work on this @Dosant! 🙏

A couple of questions. Is the comment still needed?

Also, to clarify - are you saying that if the 'icon' is a component then it would not render correctly?
I believe this component is currently only used with icon types as strings (e.g. 'pencil') passed in. Should it be updated to not allow icon components to be passed in?
cc @qn895 for anything I may have missed.

Hi! Thanks for looking.

Also, to clarify - are you saying that if the 'icon' is a component then it would not render correctly?

Yes!

The type of component allows ComponentType but it isn't handled properly during rendering. I put a TODO to call this out. If the ComponentType is not intended to be used, then the easier fix would be indeed to exclude it from the props. I'll take a look

I updated 👍

Dosant · 2024-09-06T13:25:27Z

...plications/app_search/components/credentials/credentials_flyout/form_components/key_type.tsx

        options={tokenOptions}
        value={activeApiToken.type}
        onChange={(e) => setTokenType(e.target.value)}
-        placeholder={i18n.translate(


See explanation in the PR description

Dosant · 2024-09-06T13:26:04Z

...se_search/public/applications/enterprise_search_relevance/components/inference_endpoints.tsx

        restrictWidth={false}
        customPageSections
        bottomBorder="extended"
-        docLink="inference_endpoints"


This prop didn't exist, hope I am not missing anything

Dosant · 2024-09-06T13:26:34Z

...lic/applications/fleet/sections/settings/components/edit_output_flyout/output_form_kafka.tsx

          data-test-subj="settingsOutputsFlyout.kafkaVersionInput"
          {...inputs.kafkaVersionInput.props}
          options={kafkaVersionOptions}
-          placeholder={i18n.translate(


See explanation in the PR description

Dosant · 2024-09-06T13:27:15Z

.../public/application/components/pipeline_editor/components/processor_form/processors/grok.tsx

            <DragAndDropTextList
              label={fieldsConfig.patterns.label!}
-              helpText={fieldsConfig.patterns.helpText}
+              helpText={


this is follow-up to #192083
I missed it in the pr

Dosant · 2024-09-06T13:33:24Z

x-pack/plugins/observability_solution/infra/public/components/lens/lens_chart.tsx

                      />
                    </p>
-                    <p>{missingFieldsMessage && missingFieldsMessage.longMessage}</p>
+                    <p>


This is worth a follow-up (or feel free to help in this PR)

The longMessage is typed as React.ReactNode | ((closePopover: () => void) => React.ReactNode); but used in the most places simply as a ReactNode, which is incorrect

Ideally, this should simply be ReactNode or a typeof === function case should be handled with every render.

Dosant · 2024-09-06T13:34:39Z

x-pack/test/functional_with_es_ssl/plugins/cases/public/application.tsx

  const selectCaseModal = cases.hooks.useCasesAddToExistingCaseModal();

  return (
-    <EuiPageTemplate template="empty">


this prop didn't exist

Dosant · 2024-09-09T13:47:50Z

x-pack/plugins/cases/public/components/system_actions/cases/cases_params.tsx

        fullWidth
        id="timeWindow"
-        error={errors.timeWindow}
+        error={errors.timeWindow as string[]}


A bunch of dummy casting is caused by this alerting error object that is typed as

export interface RuleFormParamsErrors { [key: string]: string | string[] | RuleFormParamsErrors; }

not sure if there is a better short term fix :(

This should be fine, in practice this is only ever string[] 👍

elasticmachine · 2024-09-09T13:48:07Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

jeramysoucy

Kibana security changes LGTM

criamico

Fleet changes LGTM

szwarckonrad

DW/Osquery changes LGTM. Thanks!

szwarckonrad · 2024-09-10T13:57:47Z

...ins/security_solution/public/management/components/artifact_list_page/artifact_list_page.tsx

              itemComponentProps={handleCardProps}
              onChange={handlePaginationChange}
-              error={error}
+              error={error as React.ReactNode}


Perhaps consider tightening the type for the component props, as it seems that error on this side isn’t intended to be a ReactNode.

# Conflicts: # x-pack/plugins/lens/public/datasources/form_based/utils.test.tsx

alvarezmelissa87

LGTM

eokoneyo

Shared UX changes LGTM

awahab07

Obs UX Logs changes LGTM!

Dosant · 2024-09-11T15:16:43Z

@elasticmachine merge upstream

Dosant · 2024-09-11T21:03:39Z

@elasticmachine merge upstream

Dosant · 2024-09-12T08:14:00Z

@elasticmachine merge upstream

kibana-ci · 2024-09-12T09:12:04Z

💚 Build Succeeded

Buildkite Build
Commit: 3a9e25a
Storybooks Preview
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-192266-3a9e25af3390

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`aiops`	547.4KB	547.5KB	+40.0B
`apm`	3.5MB	3.5MB	-38.0B
`dataVisualizer`	725.2KB	725.1KB	-21.0B
`enterpriseSearch`	2.6MB	2.6MB	-166.0B
`fleet`	1.8MB	1.8MB	-410.0B
`infra`	1.6MB	1.6MB	-11.0B
`ingestPipelines`	375.1KB	375.3KB	+156.0B
`sessionView`	392.0KB	392.1KB	+4.0B
`slo`	852.3KB	852.3KB	-1.0B
total			-447.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`fleet`	169.8KB	169.8KB	+16.0B

History

💔 Build #233605 failed a7544ad
💔 Build #233475 failed 00ae6d5
💔 Build #233400 failed 2303f55
💔 Build #233124 failed ad66440
💛 Build #233030 was flaky 3477c84

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Dosant

logeekal

LGTM from investigations side. It is a bummer we have to do these castings but thanks for doing it. Just had a small question.

logeekal · 2024-09-12T11:21:14Z

x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx

  );

-  const onAlertTableUpdate: AlertsTableStateProps['onUpdate'] = useCallback(
+  const onAlertTableUpdate = useCallback<NonNullable<AlertsTableStateProps['onUpdate']>>(


May I ask why NonNullable is needed here and what was the problem with previous type?

Sure! When assigned like this const onAlertTableUpdate: AlertsTableStateProps['onUpdate']
the argument in the function was implicitly any

to fix it, we assign like this: const onAlertTableUpdate = useCallback<AlertsTableStateProps['onUpdate']>

But this gives an error because AlertsTableStateProps['onUpdate'] could be undefined and it can't be used as a generic param. So with NonNullable we exclude undefined from that type

Perfect. Thanks for explanation. I understand now.

fixes

3c572fa

Dosant added chore release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// labels Sep 6, 2024

Dosant self-assigned this Sep 6, 2024

Dosant commented Sep 6, 2024

View reviewed changes

fixes

aa07b15

Dosant changed the title ~~[react@18] Breaking type fixes~~ [react@18] More breaking type fixes Sep 6, 2024

Dosant changed the title ~~[react@18] More breaking type fixes~~ [react@18] More breaking type fixes (should be the last) Sep 9, 2024

Dosant added 3 commits September 9, 2024 13:46

Merge branch 'main' of github.com:elastic/kibana into react18/as-fixes

5030223

fixes

8c44434

Merge branch 'main' of github.com:elastic/kibana into react18/as-fixes

29f6792

Dosant commented Sep 9, 2024

View reviewed changes

Dosant marked this pull request as ready for review September 9, 2024 13:48

Dosant requested a review from a team as a code owner September 9, 2024 13:48

Dosant requested review from a team as code owners September 9, 2024 13:48

Dosant requested a review from a team September 9, 2024 13:48

Dosant requested review from a team as code owners September 9, 2024 13:48

Dosant requested review from alvarezmelissa87 and criamico September 10, 2024 11:13

jeramysoucy approved these changes Sep 10, 2024

View reviewed changes

criamico approved these changes Sep 10, 2024

View reviewed changes

szwarckonrad approved these changes Sep 10, 2024

View reviewed changes

jpdjere approved these changes Sep 10, 2024

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into react18/as-fixes

ad66440

# Conflicts: # x-pack/plugins/lens/public/datasources/form_based/utils.test.tsx

markov00 approved these changes Sep 10, 2024

View reviewed changes

alvarezmelissa87 approved these changes Sep 11, 2024

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into react18/as-fixes

2303f55

angorayc approved these changes Sep 11, 2024

View reviewed changes

eokoneyo approved these changes Sep 11, 2024

View reviewed changes

awahab07 approved these changes Sep 11, 2024

View reviewed changes

neptunian approved these changes Sep 11, 2024

View reviewed changes

Merge branch 'main' into react18/as-fixes

00ae6d5

Merge branch 'main' into react18/as-fixes

a7544ad

Merge branch 'main' into react18/as-fixes

3a9e25a

logeekal approved these changes Sep 12, 2024

View reviewed changes

Dosant merged commit 240c4ff into elastic:main Sep 12, 2024

kibanamachine added v8.16.0 backport:skip This PR does not require backporting labels Sep 12, 2024

Dosant mentioned this pull request Sep 12, 2024

Upgrade to React@18 #138222

Closed

szaffarano mentioned this pull request Sep 13, 2024

[Telemetry][Security Solution] Fix integration tests on branch 9.0 #192842

Merged

1 task

Dosant added the React@18 label Nov 6, 2024

Conversation

Dosant commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

1 Objects are no longer considered a valid ReactNode

2 More useCallback implicit any fixes

3 EuiSelect doesn't have a placeholder prop

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 9, 2024

Uh oh!

jeramysoucy left a comment

Choose a reason for hiding this comment

Uh oh!

criamico left a comment

Choose a reason for hiding this comment

Uh oh!

szwarckonrad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

Uh oh!

eokoneyo left a comment

Choose a reason for hiding this comment

Uh oh!

awahab07 left a comment

Choose a reason for hiding this comment

Uh oh!

Dosant commented Sep 11, 2024

Uh oh!

Dosant commented Sep 11, 2024

Uh oh!

Dosant commented Sep 12, 2024

Uh oh!

kibana-ci commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

Uh oh!

logeekal left a comment

Choose a reason for hiding this comment

Uh oh!

logeekal Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Dosant commented Sep 6, 2024 •

edited

Loading

2 More `useCallback` implicit any fixes

3 `EuiSelect` doesn't have a placeholder prop

kibana-ci commented Sep 12, 2024 •

edited

Loading

logeekal Sep 12, 2024 •

edited

Loading

Dosant Sep 12, 2024 •

edited

Loading