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.

Sign up for GitHub

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

Jump to bottom

[SharedUX/React render] prevent double-nested EuiProvider #182005

Merged
merged 13 commits into from
Apr 30, 2024
Merged

[SharedUX/React render] prevent double-nested EuiProvider #182005

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
039132d
[SharedUX/React render] prevent double-nested EuiProvider
tsullivan Apr 29, 2024
081271e
Update wording
tsullivan Apr 29, 2024
1ecb97d
feedback
tsullivan Apr 29, 2024
bd5a099
Merge branch 'main' into sharedux/use-is_nested_eui_provider
tsullivan Apr 29, 2024
0ec663b
update limits
tsullivan Apr 29, 2024
58e805b
Revert "update limits"
tsullivan Apr 29, 2024
dd3f12a
add useIsNestedEuiProvider to kbn-ui-shared-deps
tsullivan Apr 29, 2024
9de2153
chore(NA): expose EUI internal component UseIsNestedEuiProvider on sh…
mistic Apr 30, 2024
4ce6bdc
Merge branch 'main' into sharedux/use-is_nested_eui_provider
tsullivan Apr 30, 2024
b9029dd
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Apr 30, 2024
773e463
Merge branch 'main' into sharedux/use-is_nested_eui_provider
tsullivan Apr 30, 2024
ec06e0d
Merge branch 'main' into sharedux/use-is_nested_eui_provider
tsullivan Apr 30, 2024
a0c33a8
update limits bytes
tsullivan Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/kbn-optimizer/limits.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pageLoadAssetSize:
data: 454087
datasetQuality: 50624
dataViewEditor: 28082
dataViewFieldEditor: 27000
dataViewFieldEditor: 40000
dataViewManagement: 5176
dataViews: 65000
dataVisualizer: 27530
Expand All @@ -51,7 +51,7 @@ pageLoadAssetSize:
expressionLegacyMetricVis: 23121
expressionMetric: 22238
expressionMetricVis: 23121
expressionPartitionVis: 29000
expressionPartitionVis: 31000
expressionRepeatImage: 22341
expressionRevealImage: 25675
expressions: 140958
Expand Down Expand Up @@ -115,7 +115,7 @@ pageLoadAssetSize:
presentationUtil: 58834
profiling: 36694
remoteClusters: 51327
reporting: 58600
reporting: 59000
rollup: 97204
runtimeFields: 41752
savedObjects: 108518
Expand Down
31 changes: 24 additions & 7 deletions packages/react/kibana_context/root/root_provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@
* Side Public License, v 1.
*/

import type { I18nStart } from '@kbn/core-i18n-browser';
import type { AnalyticsServiceStart } from '@kbn/core-analytics-browser';
import React, { FC, PropsWithChildren } from 'react';

import type { AnalyticsServiceStart } from '@kbn/core-analytics-browser';
import type { I18nStart } from '@kbn/core-i18n-browser';

// @ts-expect-error EUI exports this component internally, but Kibana isn't picking it up its types
import { useIsNestedEuiProvider } from '@elastic/eui/lib/components/provider/nested';
Copy link
Contributor

Choose a reason for hiding this comment

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

@cee-chen Can this be something addressed in a future EUI release? Or is this a mistake somehow, (I don't see how).

Copy link
Member

@cee-chen cee-chen Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not a mistake, this has been done other places in Kibana as well (whenever devs reach into @elastic/eui/lib). TBH I'm not 100% sure it's solvable on just the EUI level - I think it's a weird factor of how Kibana optimizes shared dependencies and its bazel pipeline. We might need to loop in KibanaOps for this as I'm not too sure why this happens either - I do believe it used to work at some point.

// @ts-expect-error EUI exports this component internally, but Kibana isn't picking it up its types
import { emitEuiProviderWarning } from '@elastic/eui/lib/services/theme/warning';
Copy link
Member

Choose a reason for hiding this comment

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

Did we look into adding this to kbn-ui-shared-deps?

Copy link
Member Author

@tsullivan tsullivan Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the suggestion. I am looking into it now: dd3f12a

Copy link
Member

Choose a reason for hiding this comment

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

TIL this is an option - going to bookmark this for future reference 👀


import { KibanaEuiProvider, type KibanaEuiProviderProps } from './eui_provider';

/** Props for the KibanaRootContextProvider */
Expand Down Expand Up @@ -38,8 +44,19 @@ export const KibanaRootContextProvider: FC<PropsWithChildren<KibanaRootContextPr
children,
i18n,
...props
}) => (
<KibanaEuiProvider {...props}>
<i18n.Context>{children}</i18n.Context>
</KibanaEuiProvider>
);
}) => {
const hasEuiProvider = useIsNestedEuiProvider();

if (hasEuiProvider) {
emitEuiProviderWarning(
'KibanaRootContextProvider has likely been nested in this React tree, either by direct reference or by KibanaRenderContextProvider. The result of this nesting is a nesting of EuiProvider, which has negative effects. Check your React tree for nested Kibana context providers.'
);
return <i18n.Context>{children}</i18n.Context>;
} else {
return (
<KibanaEuiProvider {...props}>
<i18n.Context>{children}</i18n.Context>
</KibanaEuiProvider>
);
}
};
4 changes: 2 additions & 2 deletions packages/react/kibana_context/theme/theme_provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import useObservable from 'react-use/lib/useObservable';

import { EuiThemeProvider, EuiThemeProviderProps } from '@elastic/eui';

// @ts-ignore EUI exports this component internally, but Kibana isn't picking it up its types
// @ts-expect-error EUI exports this component internally, but Kibana isn't picking it up its types
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch.

import { useIsNestedEuiProvider } from '@elastic/eui/lib/components/provider/nested';
// @ts-ignore EUI exports this component internally, but Kibana isn't picking it up its types
// @ts-expect-error EUI exports this component internally, but Kibana isn't picking it up its types
import { emitEuiProviderWarning } from '@elastic/eui/lib/services/theme/warning';

import { KibanaEuiProvider } from '@kbn/react-kibana-context-root';
Expand Down