fix: Simplify EuiFlex generic types #7792
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to resolve when used with wrappers like `styled()`
tkajtoch
commented
May 28, 2024
| @@ -155,6 +156,10 @@ generator.then(() => { | |||
| } | |||
| ) // end 3. | |||
| .replace(/$/, `\n\n${buildEuiTokensObject()}`) // 4. | |||
| .replaceAll( | |||
There was a problem hiding this comment.
dts-generator likes to expand all types and for some reason this causes longer type checking time
tkajtoch
commented
May 28, 2024
| @@ -51,9 +52,7 @@ export const DIRECTIONS = [ | |||
| ] as const; | |||
| type FlexGroupDirection = (typeof DIRECTIONS)[number]; | |||
|
|
|||
| type ComponentPropType = ElementType<CommonProps>; | |||
There was a problem hiding this comment.
ElementType<CommonProps> caused the global attributes to not be added to the final type
tkajtoch
commented
May 28, 2024
| >( | ||
| props: EuiFlexGroupProps<TComponent> & { | ||
| ref?: Ref<TComponentRef>; | ||
| } | ||
| ) => ReturnType<typeof EuiFlexGroupInternal>) & { displayName?: string }; | ||
| ) => ReactElement) & { displayName?: string }; |
There was a problem hiding this comment.
This loosens the return type but shouldn't have any impact on consumers of these two components. In case a stricter type is needed for ref usage, there's the TComponentRef generic argument they may override
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
cc @tkajtoch |
cee-chen
approved these changes
May 28, 2024
tkajtoch
added a commit
to elastic/kibana
that referenced
this pull request
May 29, 2024
`v94.5.1` ⏩ `v94.5.2` _[Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_ --- ## [`v94.5.2`](https://github.com/elastic/eui/releases/v94.5.2) **Bug fixes** - Fixed `EuiDatePicker` to more gracefully handle incorrectly formatted `selected` Moment dates, instead of simply crashing ([#7784](elastic/eui#7784)) - Fixed `EuiFlexGroup` and `EuiFlexItem` types to correctly accept global attribute props and simplify type resolution when used with `styled()`-like wrappers ([#7792](elastic/eui#7792))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #7783
This PR optimizes types used in
EuiFlexItemandEuiFlexGroupto be resolved faster and more reliably.We recently found out that the introduction of #7688 caused a significant increase in type-checking times in Kibana. The reason wasn't clear at first, but profiling
tscand running type checking with various type simplifications highlighted the primary error. The generic typing of these two components isn't at fault. It's actually the type inferencing and distributive conditionals used multiple times in the type resolution path and EUI types being combined into a single bigeui.d.tsall added to the type checking execution time.This PR addresses all of these by simplifying these types and fixes a bug where some global attributes (e.g.,
title) weren't recognized as a proper argument.This PR shouldn't introduce any breaking changes since the new types can be considered less strict / looser.
QA
yarn kbn bootstrapandtime node scripts/type_check --clean-cacheyarn kbn bootstrap --no-validateandtime node scripts/type_check --clean-cacheagain to confirm reduced type checking timesGeneral checklist