Upgrade to React@18 #138222
Assignees
Labels
Team:Core
Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Team:Operations
Team label for Operations Team
Team:SharedUX
Team label for AppEx-SharedUX (formerly Global Experience)
Comments
|
Pinging @elastic/kibana-operations (Team:Operations) |
|
Dropping the operations team, we have a limited intersection with client code and aren't best suited for managing this upgrade. |
jbudz
added
Team:SharedUX
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
jbudz
removed
the
Team:SharedUX
wayneseymour
added
the
Team:Core
|
Pinging @elastic/kibana-core (Team:Core) |
|
@stephmilovic just raised our attention to the problems created by the false positive errors for the setState on unmounted components facebook/react#22114 (comment) While react does have no owner so far, let's see where we can get with a small exploration to address this once we have some time. As a short term fix, at least filtering those errors for the current version we are in would be helpful. |
10 tasks
|
We should consider prioritizing this, now that React 19 is in beta: https://19.react.dev/. |
|
@legrego then I think the mandatory first step would be to find an actual owner for this issue. Atm, neither @elastic/kibana-operations nor @elastic/kibana-core agreed to take ownership on the issue, Operations, because, I quote
And Core, because, I quote myself
I'll add the @elastic/appex-sharedux team in the look, given their label was removed from the issue without any comment or explanation, even if it feels like they might potentially be the right owner for the job? |
pgayvallet
added
the
Team:SharedUX
|
@pgayvallet - Thanks Pierre! I agree, this is right in our wheelhouse. I pinged the tech leads on our public channel to start a chat about how best to get this rolling. |
|
React v19 will be released soon, with lots of great features. However, before that we need to upgrade to v18 first. React v18.3 version is available now, which is the same as v18.2 but will show a bunch of warnings and deprecations, which need to be solved before upgrading to v19.
For Kibana it means that the upgrade plan could be as follows:
|
This was referenced Aug 26, 2024
Dosant
added a commit
that referenced
this issue
Aug 28, 2024
## Summary Prep work for React@18 bump #138222 In React@18 `useCallback` types has changed that introduced breaking changes: DefinitelyTyped/DefinitelyTyped#46691 Fixed using: https://github.com/eps1lon/types-react-codemod?tab=readme-ov-file#usecallback-implicit-any **Tried to do my best with fixing the types, but if you disagree or have a better idea how it should be solved feel free to suggest changes or commit directly to the branch 🙏** --------- Co-authored-by: Sergi Massaneda <sergi.massaneda@elastic.co> Co-authored-by: Sébastien Loix <sabee77@gmail.com> Co-authored-by: Nick Peihl <nick.peihl@elastic.co> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Felix Stürmer <felix.stuermer@elastic.co> Co-authored-by: nickofthyme <nicholas.partridge@elastic.co> Co-authored-by: Davis McPhee <davis.mcphee@elastic.co> Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com> Co-authored-by: Maxim Palenov <maxim.palenov@elastic.co> Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co> Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
This was referenced Sep 4, 2024
Dosant
added a commit
that referenced
this issue
Sep 4, 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](#182344) 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. --------- Co-authored-by: Jatin Kathuria <jtn.kathuria@gmail.com>
|
We are currently working on upgrading to React 18. The new version has a compatibility mode, allowing us to upgrade the package without immediately switching to the new runtime. We have briefly tried this mode with Kibana, and while it mostly works, there are still some runtime issues that need to be addressed. However in @types/react@18 a lot of improvements and fixes have been made that caused a lot of type failures in Kibana, so we need to address or mute those first before doing the package upgrade. We’re already halfway through and there is light at the end of the tunnel. So we propose we work on the upgrade in two stages: Stage 1: Upgrade the packages to ~18.2.0 Upgrade While upgrading, fix any breaking type issues. We are already making great progress on this (thanks to @patrykkopycinski for making a lot of progress):
Address any runtime issues caused by the upgrade:
After completing these steps, we will have upgraded to React 18 packages while still running in the "old" React 17 mode. Stage 2: Switching to the New React 18 Runtime The second stage involves adopting the new APIs to switch to the new React 18 runtime. The current plan is for each team to handle this transition independently for their specific apps and components. We will look into this stage in more detail as we get closer and provide some guidance for the teams. |
eokoneyo
added a commit
that referenced
this issue
Sep 5, 2024
## Summary This PR fixes an error that was discovered ahead of the react 18 upgrade(#138222). Why are we concerned about this? Whilst `useId` isn't directly used in our codebase, it is utilized by EUI's `useGeneratedHtmlId` hook, see https://github.com/elastic/eui/blob/v95.8.0/packages/eui/src/services/accessibility/html_id_generator.ts#L73-L74, the useId hook returns a string that is bounded by colons; if passed to `document.querySelector` as an ID selector, they would cause errors (see facebook/react#26839). As a result, instances where these IDs are being used with `querySelector` have been modified to instead use an attribute selector equivalent to the corresponding ID selector. <!-- ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --> --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Dosant
added a commit
that referenced
this issue
Sep 6, 2024
## Summary Part of #138222 This is one of the issues that `@types/react@18` upgrade highlights and that needs to be addressed with or before the upgrade. `field.helpText` is `ReactNode`, a function is not a valid `ReactNode`, but `@types/react@17` allowed a function. That is why `typeof field.helpText === 'function' ? field.helpText() : field.helpText` doesn't fail with `@17`, but fails with `@18` as `field.helpText is not callable, as never is not callable`. You can check the types with @types/react@18 yourself here #191738 It looks like the lazy `helpText` isn't needed apart from a hack for index management where documentation service is used as part of helpText that could be not available yet. To keep it supported, I specifically, allowed a function that returns a ReactNode for `helpText` on `FieldConfig`, but to make consumption clean and to now break other places `useField` will call it and pass just a `ReactNode` down
Dosant
added a commit
that referenced
this issue
Sep 9, 2024
## Summary Part of #138222 in @types/react@18 types [have become more strict](DefinitelyTyped/DefinitelyTyped#56210). This PR addresses a bunch of easy fixes. The most common are: ### 1 Removal of implicit children For components that do implement children but relied on their implicit declaration from React.FunctionComponent or React.Component: ```diff interface Props { + children?: React.ReactNode; } class SomeClassComponents React.Component<Props> { render() { return <div>{this.props.children}</div> } } const SomeFunctionComponent: React.FunctionComponent<Props> = props => <div>{props.children}</div> ``` or ```diff - const SomeFunctionComponent: React.FunctionComponent<Props> = props => <div>{props.children}</div> + const SomeFunctionComponent: React.FunctionComponent<React.PropsWithChildren<Props>> = props => <div>{props.children}</div> ``` *Note 1:* The most common change occurs in unit tests where `renderHook` and `wrapper` are used. I had to re-type the props so that `children` were there ```diff const { result } = renderHook( () => { return useLicense(); }, { - wrapper: ({ children }) => ( + wrapper: ({ children }: React.PropsWithChildren<{}>) => ( <TestProviders license={license}>{children}</TestProviders> ), } ); ``` ```diff - const { result } = renderHook<GetCasesColumn, UseCasesColumnsReturnValue>( + const { result } = renderHook<React.PropsWithChildren<GetCasesColumn>, UseCasesColumnsReturnValue>( () => useCasesColumns(defaultColumnArgs), { wrapper: ({ children }) => <TestProviders>{children}</TestProviders>, } ); ``` *Note 2:* With @types/react@17 the components that don't have any props apart from `children` I had to use `React.PropsWithChildren<{}>`, whereas in @types/react@18 the argument becomes optional, so it can be omitted, and type simpler with `React.PropsWithChildren` without an argument ### 2 `this.context` becomes `unknown` (was `any`) In a couple of places where `this.context` is used, I had to type it ### 3 `ComponentType` from enzyme is no longer compatible with `ComponentType` from react This one is a bummer, but where `wrappingComponent` with enzyme is used I had to cast it to type from `enzyme` ```diff - import { mount } from 'enzyme' + import { mount, ComponentType } from 'enzyme' wrapper = mount(<ClosureOptions {...props} />, { - wrappingComponent: TestProviders, + wrappingComponent: TestProviders as ComponentType<{}>, }); ```
Dosant
added a commit
that referenced
this issue
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
gergoabraham
pushed a commit
to gergoabraham/kibana
that referenced
this issue
Sep 13, 2024
## Summary Part of elastic#138222 in @types/react@18 types [have become more strict](DefinitelyTyped/DefinitelyTyped#56210). This PR addresses a bunch of easy fixes. The most common are: ### 1 Removal of implicit children For components that do implement children but relied on their implicit declaration from React.FunctionComponent or React.Component: ```diff interface Props { + children?: React.ReactNode; } class SomeClassComponents React.Component<Props> { render() { return <div>{this.props.children}</div> } } const SomeFunctionComponent: React.FunctionComponent<Props> = props => <div>{props.children}</div> ``` or ```diff - const SomeFunctionComponent: React.FunctionComponent<Props> = props => <div>{props.children}</div> + const SomeFunctionComponent: React.FunctionComponent<React.PropsWithChildren<Props>> = props => <div>{props.children}</div> ``` *Note 1:* The most common change occurs in unit tests where `renderHook` and `wrapper` are used. I had to re-type the props so that `children` were there ```diff const { result } = renderHook( () => { return useLicense(); }, { - wrapper: ({ children }) => ( + wrapper: ({ children }: React.PropsWithChildren<{}>) => ( <TestProviders license={license}>{children}</TestProviders> ), } ); ``` ```diff - const { result } = renderHook<GetCasesColumn, UseCasesColumnsReturnValue>( + const { result } = renderHook<React.PropsWithChildren<GetCasesColumn>, UseCasesColumnsReturnValue>( () => useCasesColumns(defaultColumnArgs), { wrapper: ({ children }) => <TestProviders>{children}</TestProviders>, } ); ``` *Note 2:* With @types/react@17 the components that don't have any props apart from `children` I had to use `React.PropsWithChildren<{}>`, whereas in @types/react@18 the argument becomes optional, so it can be omitted, and type simpler with `React.PropsWithChildren` without an argument ### 2 `this.context` becomes `unknown` (was `any`) In a couple of places where `this.context` is used, I had to type it ### 3 `ComponentType` from enzyme is no longer compatible with `ComponentType` from react This one is a bummer, but where `wrappingComponent` with enzyme is used I had to cast it to type from `enzyme` ```diff - import { mount } from 'enzyme' + import { mount, ComponentType } from 'enzyme' wrapper = mount(<ClosureOptions {...props} />, { - wrappingComponent: TestProviders, + wrappingComponent: TestProviders as ComponentType<{}>, }); ```
gergoabraham
pushed a commit
to gergoabraham/kibana
that referenced
this issue
Sep 13, 2024
…192266) ## Summary Part of elastic#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 elastic#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
markov00
pushed a commit
to markov00/kibana
that referenced
this issue
Sep 18, 2024
…192266) Part of elastic#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: 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. Follow up to elastic#191659 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
added a commit
that referenced
this issue
Sep 20, 2024
## Summary Prep for #138222 This PR updates `styled-components` package to the latest v5 version. I would like to do it for react@18 upgrade to suppress a noisy warning that is coming from `styled-components` when it is used with react@18. Here is that PR https://github.com/styled-components/styled-components/pull/3564/files Updates the failed snapshot tests. Dear reviewers, could you please smoke-check your apps visually? This is a minor upgrade, but I think it is worth to click around anyway.
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this issue
Sep 20, 2024
## Summary Prep for elastic#138222 This PR updates `styled-components` package to the latest v5 version. I would like to do it for react@18 upgrade to suppress a noisy warning that is coming from `styled-components` when it is used with react@18. Here is that PR https://github.com/styled-components/styled-components/pull/3564/files Updates the failed snapshot tests. Dear reviewers, could you please smoke-check your apps visually? This is a minor upgrade, but I think it is worth to click around anyway. (cherry picked from commit b1434a4)
kibanamachine
added a commit
that referenced
this issue
Sep 20, 2024
# Backport This will backport the following commits from `main` to `8.x`: - [[react@18] bump `styled-components` (#192368)](#192368) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Anton Dosov","email":"anton.dosov@elastic.co"},"sourceCommit":{"committedDate":"2024-09-20T09:26:47Z","message":"[react@18] bump `styled-components` (#192368)\n\n## Summary\r\n\r\nPrep for #138222 PR updates `styled-components` package to the latest v5 version. I\r\nwould like to do it for react@18 upgrade to suppress a noisy warning\r\nthat is coming from `styled-components` when it is used with react@18.\r\nHere is that PR\r\nhttps://github.com/styled-components/styled-components/pull/3564/files\r\n\r\n\r\nUpdates the failed snapshot tests.\r\n\r\n\r\nDear reviewers, could you please smoke-check your apps visually? This is\r\na minor upgrade, but I think it is worth to click around anyway.","sha":"b1434a446fecd1feae4230e0e9a4fbe0fea4d8f1","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:SharedUX","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-infra_services","apm:review"],"title":"[react@18] bump `styled-components`","number":192368,"url":"#192368 bump `styled-components` (#192368)\n\n## Summary\r\n\r\nPrep for #138222 PR updates `styled-components` package to the latest v5 version. I\r\nwould like to do it for react@18 upgrade to suppress a noisy warning\r\nthat is coming from `styled-components` when it is used with react@18.\r\nHere is that PR\r\nhttps://github.com/styled-components/styled-components/pull/3564/files\r\n\r\n\r\nUpdates the failed snapshot tests.\r\n\r\n\r\nDear reviewers, could you please smoke-check your apps visually? This is\r\na minor upgrade, but I think it is worth to click around anyway.","sha":"b1434a446fecd1feae4230e0e9a4fbe0fea4d8f1"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"#192368 bump `styled-components` (#192368)\n\n## Summary\r\n\r\nPrep for #138222 PR updates `styled-components` package to the latest v5 version. I\r\nwould like to do it for react@18 upgrade to suppress a noisy warning\r\nthat is coming from `styled-components` when it is used with react@18.\r\nHere is that PR\r\nhttps://github.com/styled-components/styled-components/pull/3564/files\r\n\r\n\r\nUpdates the failed snapshot tests.\r\n\r\n\r\nDear reviewers, could you please smoke-check your apps visually? This is\r\na minor upgrade, but I think it is worth to click around anyway.","sha":"b1434a446fecd1feae4230e0e9a4fbe0fea4d8f1"}}]}] BACKPORT--> --------- Co-authored-by: Anton Dosov <anton.dosov@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The text was updated successfully, but these errors were encountered: