Skip to content

[react@18] (Part 2) fix useCallback breaking type changes#191659

Merged
Dosant merged 15 commits intoelastic:mainfrom
Dosant:react18/callback-any-2
Sep 4, 2024
Merged

[react@18] (Part 2) fix useCallback breaking type changes#191659
Dosant merged 15 commits intoelastic:mainfrom
Dosant:react18/callback-any-2

Conversation

@Dosant
Copy link
Contributor

@Dosant Dosant commented Aug 28, 2024

Summary

This is a prep for #138222 and follow up to #182344. These are post-merge leftovers and new instances from the previous run

In React@18 useCallback types have changed that introducing breaking changes:
DefinitelyTyped/DefinitelyTyped#46691

Found potential issues using:
https://github.com/eps1lon/types-react-codemod?tab=readme-ov-file#usecallback-implicit-any

I tried to do my best to fix the type where possible, but there are some complicated cases where I kept any after some struggling. Please feel free to push improvements directly.

@Dosant Dosant changed the title React18/callback any 2 [react@18] More fix useCallback breaking type changes (part 2) Aug 28, 2024
@Dosant Dosant changed the title [react@18] More fix useCallback breaking type changes (part 2) [react@18] (Part 2) fix useCallback breaking type changes Aug 28, 2024
@Dosant
Copy link
Contributor Author

Dosant commented Aug 29, 2024

/ci

@Dosant Dosant added chore release_note:skip Skip the PR/issue when compiling release notes labels Aug 29, 2024
@Dosant Dosant marked this pull request as ready for review August 29, 2024 09:43
@Dosant Dosant requested review from a team as code owners August 29, 2024 09:43
@Dosant Dosant requested a review from a team August 29, 2024 09:43
@Dosant Dosant requested review from a team as code owners August 29, 2024 09:43
@Dosant
Copy link
Contributor Author

Dosant commented Sep 3, 2024

@elasticmachine merge upstream

Copy link
Contributor

@achyutjhunjhunwala achyutjhunjhunwala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infra and Logs Shared plugin changes LGTM 👍🏼

@Dosant
Copy link
Contributor Author

Dosant commented Sep 3, 2024

@elasticmachine merge upstream

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseOps changes LGTM. I left some comments (#191659 (comment) and #191659 (comment)) on how to fix some of the any.

Copy link
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a Fix couple of types. Please check. LGTM from investigation side. Thank you for this fix 🚀

const render = useCallback(
(dataProvider, _, snapshot) =>
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(dataProvider: any, _: any, snapshot: any) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here : 8f4d128

const render = useCallback(
(_props, _provided, snapshot) =>
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(_props: any, _provided: any, snapshot: any) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here : 8f4d128

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

@ilyannn
Copy link
Contributor

ilyannn commented Sep 3, 2024

Scalability LGTM

…k-any-2

# Conflicts:
#	packages/kbn-unified-data-table/src/components/data_table.tsx
@Dosant
Copy link
Contributor Author

Dosant commented Sep 4, 2024

@elasticmachine merge upstream

@Dosant Dosant enabled auto-merge (squash) September 4, 2024 13:08
@kibana-ci
Copy link

kibana-ci commented Sep 4, 2024

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: 9803343
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-191659-9803343b24e6

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #23 / machine learning - data visualizer esql data visualizer with farequote ES|QL farequote displays index details

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
discover 794.6KB 794.6KB +4.0B
esql 178.1KB 178.2KB +74.0B
securitySolution 20.7MB 20.7MB -7.0B
total +71.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/elastic-assistant 15 21 +6
securitySolution 537 538 +1
total +7

Total ESLint disabled count

id before after diff
@kbn/elastic-assistant 16 22 +6
securitySolution 622 623 +1
total +7

History

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

@Dosant Dosant merged commit 8199dd0 into elastic:main Sep 4, 2024
@kibanamachine kibanamachine added v8.16.0 backport:skip This PR does not require backporting labels Sep 4, 2024
@Dosant Dosant self-assigned this Sep 4, 2024
Dosant added a commit that referenced this pull request Sep 12, 2024
## Summary

Part of #138222

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)


```diff

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 Dosant added the React@18 label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

apm:review backport:skip This PR does not require backporting chore ci:project-deploy-observability Create an Observability project React@18 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comments