Skip to content

Commit 584875e

Browse files
[Core] Update RenderingService withaddContext public method (elastic#212163)
## Summary Epic: elastic/kibana-team#1435 Closes elastic#205413 Closes elastic#205411 This PR creates a new way to expose stateful service dependencies needed for rendering React elements in Kibana. The concept of the changes is that `KibanaRenderContextProvider` should not be a shared module, but should be wrapped by a core service (the `RenderContextService` name is TBD). The next steps in this direction would be to coordinate teams to migrate away from directly using `KibanaRenderContextProvider`. ### Background Today, the dependencies for `KibanaRenderContextProvider` are declared as separate services which can be found in the `CoreStart` context. This has created a situation where enhancing the module with more dependencies creates widespread changes needed for the UI codebase. The @elastic/appex-sharedux team is looking to solve this situation by defining a less impactful way to make future enhancements. The solution is one that can be gradually migrated towards, so the SharedUX team can ask Kibana contributors to migrate towards in their own code. This PR offers a POC for that solution. ### Details of this POC The driving goal for this refactor is to lessen the impact across the Kibana codebase whenever the `KibanaRenderContext` module needs to require additional services from the `CoreStart` context. #### Rendering a React Element with `ReactDOM.render`: Before ```tsx const renderApp = ({ core, targetDomElement }: { core: CoreStart; targetDomElement: HTMLElement; }) => { // If `KibanaRenderContextProvider` needs to expand its scope, more services could be needed here, // updating all the places throughout the code to pass those is a ton of work 👎🏻 const { i18n, theme, analytics, userProfile, executionContext } = core; ReactDOM.render( <KibanaRenderContextProvider {...{ i18n, theme, analytics, userProfile, executionContext }}> <MyApplication /> </KibanaRenderContextProvider>, targetDomElement ); return () => ReactDOM.unmountComponentAtNode(targetDomElement); }; ``` #### Rendering a React Element with `ReactDOM.render`: After ```tsx const renderApp = ({ core, targetDomElement }: { core: CoreStart; targetDomElement: HTMLElement; }) => { // So much less code, so much more future-proof 👍🏻 ReactDOM.render(core.rendering.addContext(<MyApplication />), targetDomElement); return () => ReactDOM.unmountComponentAtNode(targetDomElement); }; ``` ### Alternatives considered See elastic#209161 ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [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 ### FAQ 1. **Q**: This is React-centric. Does this give Kibana more commitment towards React? **A:** For now, yes. But if we want to Kibana to remain framework-agnostic we may be able to add more extensions to the RenderContextService that support other frameworks. 1. **Q:** Why not have a service that wraps `ReactDOM.render`? **A:** As we steer towards upgrading to React 18 in Kibana, staying agnostic of how React is rendered benefits us. React 18 has different ergonomics based on whether you want to update an existing tree or mount a new one. 1. **Q:** Does the API have to be named `rendering.addContext`? **A:** No, it does not. Please suggest a better name if you have one in mind. 1. **Q:** What are the next steps? **A:** Refer to the [Epic](elastic/kibana-team#1435). This PR started as a POC but became ready for merge. After it is delivered to the codebase, the next steps are to improve documentation and engage in "sheparding." That is, socialize the new way of injecting dependencies into the context, support teams in their migration, and track the progress of migration. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [x] Care is needed to ensure this doesn't not negatively impact performance with unnecessary re-renders. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
1 parent c2f3495 commit 584875e

39 files changed

Lines changed: 424 additions & 392 deletions

File tree

src/core/packages/chrome/browser-internal/src/handle_system_colormode_change.tsx

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
import React from 'react';
1111
import { i18n } from '@kbn/i18n';
12-
import { toMountPoint } from '@kbn/react-kibana-mount';
12+
import { mountReactNode } from '@kbn/core-mount-utils-browser-internal';
1313
import { EuiButton, EuiFlexGroup, EuiFlexItem } from '@elastic/eui';
1414
import type { NotificationsStart } from '@kbn/core-notifications-browser';
1515
import type { I18nStart } from '@kbn/core-i18n-browser';
@@ -108,7 +108,7 @@ export async function handleSystemColorModeChange({
108108
title: i18n.translate('core.ui.chrome.appearanceChange.successNotificationTitle', {
109109
defaultMessage: 'System color mode updated',
110110
}),
111-
text: toMountPoint(
111+
text: mountReactNode(
112112
<>
113113
<p>
114114
{i18n.translate('core.ui.chrome.appearanceChange.successNotificationText', {
@@ -131,8 +131,7 @@ export async function handleSystemColorModeChange({
131131
</EuiButton>
132132
</EuiFlexItem>
133133
</EuiFlexGroup>
134-
</>,
135-
coreStart
134+
</>
136135
),
137136
},
138137
{ toastLifeTimeMs: Infinity } // leave it on until discard or page reload

src/core/packages/chrome/browser-internal/tsconfig.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@
6161
"@kbn/core-user-profile-browser",
6262
"@kbn/core-ui-settings-browser",
6363
"@kbn/core-user-profile-common",
64-
"@kbn/react-kibana-mount",
6564
"@kbn/core-theme-browser-internal"
6665
],
6766
"exclude": [

src/core/packages/lifecycle/browser-mocks/src/core_start.mock.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import { chromeServiceMock } from '@kbn/core-chrome-browser-mocks';
2424
import { customBrandingServiceMock } from '@kbn/core-custom-branding-browser-mocks';
2525
import { securityServiceMock } from '@kbn/core-security-browser-mocks';
2626
import { userProfileServiceMock } from '@kbn/core-user-profile-browser-mocks';
27+
import { renderingServiceMock } from '@kbn/core-rendering-browser-mocks';
2728
import { coreFeatureFlagsMock } from '@kbn/core-feature-flags-browser-mocks';
2829

2930
export function createCoreStartMock({ basePath = '' } = {}) {
@@ -47,6 +48,7 @@ export function createCoreStartMock({ basePath = '' } = {}) {
4748
fatalErrors: fatalErrorsServiceMock.createStartContract(),
4849
security: securityServiceMock.createStart(),
4950
userProfile: userProfileServiceMock.createStart(),
51+
rendering: renderingServiceMock.create(),
5052
plugins: {
5153
onStart: jest.fn(),
5254
},

src/core/packages/lifecycle/browser-mocks/tsconfig.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@
2929
"@kbn/core-custom-branding-browser-mocks",
3030
"@kbn/core-security-browser-mocks",
3131
"@kbn/core-user-profile-browser-mocks",
32-
"@kbn/core-feature-flags-browser-mocks"
32+
"@kbn/core-feature-flags-browser-mocks",
33+
"@kbn/core-rendering-browser-mocks"
3334
],
3435
"exclude": [
3536
"target/**/*",

src/core/packages/lifecycle/browser/src/core_start.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import type { ChromeStart } from '@kbn/core-chrome-browser';
2424
import type { CustomBrandingStart } from '@kbn/core-custom-branding-browser';
2525
import type { PluginsServiceStart } from '@kbn/core-plugins-contracts-browser';
2626
import type { SecurityServiceStart } from '@kbn/core-security-browser';
27+
import type { RenderingService } from '@kbn/core-rendering-browser';
2728
import type { UserProfileServiceStart } from '@kbn/core-user-profile-browser';
2829
import type { FeatureFlagsStart } from '@kbn/core-feature-flags-browser';
2930

@@ -81,4 +82,6 @@ export interface CoreStart {
8182
security: SecurityServiceStart;
8283
/** {@link UserProfileServiceStart} */
8384
userProfile: UserProfileServiceStart;
85+
/** {@link RenderingService} */
86+
rendering: RenderingService;
8487
}

src/core/packages/lifecycle/browser/tsconfig.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@
3030
"@kbn/core-plugins-contracts-browser",
3131
"@kbn/core-security-browser",
3232
"@kbn/core-user-profile-browser",
33-
"@kbn/core-feature-flags-browser"
33+
"@kbn/core-feature-flags-browser",
34+
"@kbn/core-rendering-browser"
3435
],
3536
"exclude": [
3637
"target/**/*",

src/core/packages/mount-utils/browser-internal/src/mount.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ export const MountWrapper: MountWrapperComponent = ({ mount, className = default
3737

3838
/**
3939
* Mount converter for react node.
40+
* This should only be used in internal Core packages to prevent circular dependency issues
4041
*
4142
* @param node to get a mount for
4243
* @internal

src/core/packages/notifications/browser-internal/src/notifications_service.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,11 @@ import { i18n } from '@kbn/i18n';
1111

1212
import { Subscription } from 'rxjs';
1313
import type { AnalyticsServiceStart, AnalyticsServiceSetup } from '@kbn/core-analytics-browser';
14-
import type { ThemeServiceStart } from '@kbn/core-theme-browser';
15-
import type { UserProfileService } from '@kbn/core-user-profile-browser';
16-
import type { I18nStart } from '@kbn/core-i18n-browser';
1714
import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser';
1815
import type { OverlayStart } from '@kbn/core-overlays-browser';
1916
import type { NotificationsSetup, NotificationsStart } from '@kbn/core-notifications-browser';
2017
import type { PublicMethodsOf } from '@kbn/utility-types';
18+
import type { RenderingService } from '@kbn/core-rendering-browser';
2119
import { showErrorDialog, ToastsService } from './toasts';
2220
import { EventReporter, eventTypes } from './toasts/telemetry';
2321

@@ -27,10 +25,8 @@ export interface SetupDeps {
2725
}
2826

2927
export interface StartDeps {
30-
i18n: I18nStart;
3128
overlays: OverlayStart;
32-
theme: ThemeServiceStart;
33-
userProfile: UserProfileService;
29+
rendering: RenderingService;
3430
analytics: AnalyticsServiceStart;
3531
targetDomElement: HTMLElement;
3632
}

src/core/packages/notifications/browser-internal/src/toasts/__snapshots__/error_toast.test.tsx.snap

Lines changed: 2 additions & 41 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/core/packages/notifications/browser-internal/src/toasts/__snapshots__/toasts_service.test.tsx.snap

Lines changed: 11 additions & 52 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)