Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[react@18] fix field.helpText type issue #192083

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Sep 4, 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 Dosant force-pushed the react18/help-text-type-issue branch from 3cd6502 to 0760701 Compare September 4, 2024 13:13
@@ -211,7 +211,7 @@ export interface FieldHook<T = unknown, I = T> {
export interface FieldConfig<T = unknown, FormType extends FormData = FormData, I = T> {
readonly label?: string;
readonly labelAppend?: string | ReactNode;
readonly helpText?: string | ReactNode;
readonly helpText?: string | ReactNode | (() => ReactNode);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we allow a lazy helpText. It is already used like this in some places, it worked because a function could be assigned to ReactNode due to a mistake in @types/react@17. This is no longer allowed with @types/react@18

@@ -549,7 +549,7 @@ export const useField = <T, FormType = FormData, I = T>(
type,
label,
labelAppend,
helpText,
helpText: typeof helpText === 'function' ? helpText() : helpText,
Copy link
Contributor Author

@Dosant Dosant Sep 4, 2024

Choose a reason for hiding this comment

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

Here we're unwraping the lazy helpText so that the useField() receives the ReactNode value. This allows to simplify a bunch of existing places and not to fix a bunch of other places where helpText={helpText} is already used

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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
dataViewEditor 49.4KB 49.3KB -43.0B
indexManagement 679.7KB 679.6KB -43.0B
total -86.0B

Page load bundle

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

id before after diff
esUiShared 158.9KB 158.2KB -706.0B
triggersActionsUi 121.7KB 121.6KB -43.0B
total -749.0B

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

@Dosant Dosant added chore release_note:skip Skip the PR/issue when compiling release notes labels Sep 4, 2024
@Dosant Dosant marked this pull request as ready for review September 4, 2024 14:55
@Dosant Dosant requested review from a team as code owners September 4, 2024 14:55
@Dosant Dosant added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Sep 4, 2024
@Dosant Dosant self-assigned this Sep 4, 2024
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Code-only review, Data Discovery changes LGTM 👍

@Dosant Dosant mentioned this pull request Sep 5, 2024
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 LTGM!

Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Index Management changes lgtm!

@Dosant Dosant merged commit bec5117 into elastic:main Sep 6, 2024
26 checks passed
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Sep 6, 2024
@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
backport:skip This commit does not require backporting chore React@18 release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants