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

Fix context.pageName by fixing missing executionContext and add enableExecutionContextTracking flag #204547

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
3925c21
Fix missing executionContext
maryam-saeidi Nov 21, 2024
d0b0d51
Revert ExecutionContextStart directly
maryam-saeidi Nov 21, 2024
b0c621c
add router to bazel
Dosant Nov 21, 2024
dcc81b8
Merge pull request #4 from Dosant/195778-fix-pageName
maryam-saeidi Nov 21, 2024
68cae60
Add temporary TODOs
maryam-saeidi Nov 25, 2024
3f82e98
Merge branch 'main' into 195778-fix-pageName
maryam-saeidi Nov 25, 2024
87ae813
Revert useExecutionContext test in APM
maryam-saeidi Nov 25, 2024
89b004b
Remove TODO from src/plugins/discover/public/application/context/cont…
davismcphee Nov 25, 2024
81a712c
fix(dataset-quality): remove TODO
Nov 26, 2024
7e4df5d
Merge branch 'main' into 195778-fix-pageName
maryam-saeidi Nov 26, 2024
2a5a780
Remove comments for IM, Snapshot restore, UA
ElenaStoeva Nov 28, 2024
7c85f18
Merge branch 'main' into 195778-fix-pageName
maryam-saeidi Dec 16, 2024
a83ff91
Allow disabling default execution context tracking if there is a cust…
maryam-saeidi Dec 16, 2024
899a3fe
Change disableExecutionContextTracking to enableExecutionContextTracking
maryam-saeidi Dec 16, 2024
1fc0b38
Fix executionContext dep and enable tracking for observability
maryam-saeidi Dec 17, 2024
7a04db2
Remove comments
maryam-saeidi Dec 17, 2024
79586d6
Make executionContext dep optional
maryam-saeidi Dec 17, 2024
6ebfcf3
Merge branch 'main' into 195778-fix-pageName-second-try
maryam-saeidi Dec 17, 2024
b22b5b8
Fix test
maryam-saeidi Dec 17, 2024
e23b561
Enable execution context tracking for observability
maryam-saeidi Dec 17, 2024
9b3532c
Fix comment
maryam-saeidi Dec 17, 2024
c56ab90
Rename SharedUXContext to SharedUXRouterContext
maryam-saeidi Dec 18, 2024
53a7e9a
Throw error if executionContext is undefined
maryam-saeidi Dec 18, 2024
2e2eb1f
Merge branch 'main' into 195778-fix-pageName-second-try
maryam-saeidi Dec 18, 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
4 changes: 2 additions & 2 deletions packages/react/kibana_context/render/render_provider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ export type KibanaRenderContextProviderProps = Omit<KibanaRootContextProviderPro
export const KibanaRenderContextProvider: FC<
PropsWithChildren<KibanaRenderContextProviderProps>
> = ({ children, ...props }) => {
const { analytics, i18n, theme, userProfile, colorMode, modify } = props;
const { analytics, executionContext, i18n, theme, userProfile, colorMode, modify } = props;
return (
<KibanaRootContextProvider
globalStyles={false}
{...{ i18n, theme, userProfile, modify, colorMode }}
{...{ executionContext, i18n, theme, userProfile, modify, colorMode }}
>
<KibanaErrorBoundaryProvider analytics={analytics}>
<KibanaErrorBoundary>{children}</KibanaErrorBoundary>
Expand Down
1 change: 1 addition & 0 deletions packages/react/kibana_context/root/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ DEPS = [
"@npm//tslib",
"@npm//@elastic/eui",
"//packages/core/base/core-base-common",
"//packages/shared-ux/router/impl:shared-ux-router",
]

js_library(
Expand Down
6 changes: 6 additions & 0 deletions packages/react/kibana_context/root/root_provider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { useEuiTheme } from '@elastic/eui';
import type { UseEuiTheme } from '@elastic/eui';
import { mountWithIntl } from '@kbn/test-jest-helpers';
import type { KibanaTheme } from '@kbn/react-kibana-context-common';
import type { ExecutionContextStart } from '@kbn/core-execution-context-browser';
import { executionContextServiceMock } from '@kbn/core-execution-context-browser-mocks';
import { i18nServiceMock } from '@kbn/core-i18n-browser-mocks';
import { I18nStart } from '@kbn/core-i18n-browser';
import type { UserProfileService } from '@kbn/core-user-profile-browser';
Expand All @@ -25,11 +27,13 @@ describe('KibanaRootContextProvider', () => {
let euiTheme: UseEuiTheme | undefined;
let i18nMock: I18nStart;
let userProfile: UserProfileService;
let executionContext: ExecutionContextStart;

beforeEach(() => {
euiTheme = undefined;
i18nMock = i18nServiceMock.createStartContract();
userProfile = userProfileServiceMock.createStart();
executionContext = executionContextServiceMock.createStartContract();
});

const flushPromises = async () => {
Expand Down Expand Up @@ -64,6 +68,7 @@ describe('KibanaRootContextProvider', () => {
<KibanaRootContextProvider
i18n={i18nMock}
userProfile={userProfile}
executionContext={executionContext}
theme={{ theme$: of(coreTheme) }}
>
<InnerComponent />
Expand All @@ -82,6 +87,7 @@ describe('KibanaRootContextProvider', () => {
<KibanaRootContextProvider
i18n={i18nMock}
userProfile={userProfile}
executionContext={executionContext}
theme={{ theme$: coreTheme$ }}
>
<InnerComponent />
Expand Down
14 changes: 12 additions & 2 deletions packages/react/kibana_context/root/root_provider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import React, { FC, PropsWithChildren } from 'react';

import type { AnalyticsServiceStart } from '@kbn/core-analytics-browser';
import type { I18nStart } from '@kbn/core-i18n-browser';
import type { ExecutionContextStart } from '@kbn/core-execution-context-browser';
import { SharedUXContext } from '@kbn/shared-ux-router';

// @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';
Expand All @@ -25,6 +27,8 @@ export interface KibanaRootContextProviderProps extends KibanaEuiProviderProps {
i18n: I18nStart;
/** The `AnalyticsServiceStart` API from `CoreStart`. */
analytics?: Pick<AnalyticsServiceStart, 'reportEvent'>;
/** The `ExecutionContextStart` API from `CoreStart`. */
executionContext?: ExecutionContextStart;
}

/**
Expand All @@ -44,20 +48,26 @@ export interface KibanaRootContextProviderProps extends KibanaEuiProviderProps {
export const KibanaRootContextProvider: FC<PropsWithChildren<KibanaRootContextProviderProps>> = ({
children,
i18n,
executionContext,
...props
}) => {
const hasEuiProvider = useIsNestedEuiProvider();
const rootContextProvider = (
<SharedUXContext.Provider value={{ services: { executionContext } }}>
<i18n.Context>{children}</i18n.Context>
</SharedUXContext.Provider>
);

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>;
return rootContextProvider;
} else {
const { theme, userProfile, globalStyles, colorMode, modify } = props;
return (
<KibanaEuiProvider {...{ theme, userProfile, globalStyles, colorMode, modify }}>
<i18n.Context>{children}</i18n.Context>
{rootContextProvider}
</KibanaEuiProvider>
);
}
Expand Down
3 changes: 3 additions & 0 deletions packages/react/kibana_context/root/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,8 @@
"@kbn/core-analytics-browser",
"@kbn/core-user-profile-browser",
"@kbn/core-user-profile-browser-mocks",
"@kbn/core-execution-context-browser",
"@kbn/core-execution-context-browser-mocks",
"@kbn/shared-ux-router"
]
}
34 changes: 34 additions & 0 deletions packages/shared-ux/router/impl/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
load("@build_bazel_rules_nodejs//:index.bzl", "js_library")

SRCS = glob(
[
"**/*.ts",
"**/*.tsx",
],
exclude = [
"**/test_helpers.ts",
"**/*.config.js",
"**/*.mock.*",
"**/*.test.*",
"**/*.stories.*",
"**/__snapshots__/**",
"**/integration_tests/**",
"**/mocks/**",
"**/scripts/**",
"**/storybook/**",
"**/test_fixtures/**",
"**/test_helpers/**",
],
)

DEPS = [

]

js_library(
name = "shared-ux-router",
package_name = "@kbn/shared-ux-router",
srcs = ["package.json"] + SRCS,
deps = DEPS,
visibility = ["//visibility:public"],
)

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions packages/shared-ux/router/impl/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@
export { Route } from './route';
export { HashRouter, BrowserRouter, MemoryRouter, Router } from './router';
export { Routes } from './routes';

export { SharedUXContext } from './services';
19 changes: 19 additions & 0 deletions packages/shared-ux/router/impl/route.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,34 @@

import React, { Component, FC } from 'react';
import { shallow } from 'enzyme';
import { useSharedUXRoutesContext } from './routes_context';
import { Route } from './route';
import { createMemoryHistory } from 'history';

jest.mock('./routes_context', () => ({
useSharedUXRoutesContext: jest.fn().mockImplementation(() => ({
enableExecutionContextTracking: true,
})),
}));

describe('Route', () => {
beforeEach(() => {
jest.restoreAllMocks();
});

test('renders', () => {
const example = shallow(<Route />);
expect(example).toMatchSnapshot();
});

test('renders with enableExecutionContextTracking as false', () => {
(useSharedUXRoutesContext as jest.Mock).mockImplementationOnce(() => ({
enableExecutionContextTracking: false,
}));
const example = shallow(<Route />);
expect(example).toMatchSnapshot();
});

test('location renders as expected', () => {
// create a history
const historyLocation = createMemoryHistory();
Expand Down
10 changes: 6 additions & 4 deletions packages/shared-ux/router/impl/route.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
RouteProps,
useRouteMatch,
} from 'react-router-dom';
import { useSharedUXRoutesContext } from './routes_context';
import { useKibanaSharedUX } from './services';
import { useSharedUXExecutionContext } from './use_execution_context';

Expand All @@ -30,17 +31,18 @@ export const Route = <T extends {}>({
render,
...rest
}: RouteProps<string, { [K: string]: string } & T>) => {
const { enableExecutionContextTracking } = useSharedUXRoutesContext();
const component = useMemo(() => {
if (!Component) {
return undefined;
}
return (props: RouteComponentProps) => (
<>
<MatchPropagator />
{enableExecutionContextTracking && <MatchPropagator />}
<Component {...props} />
</>
);
}, [Component]);
}, [Component, enableExecutionContextTracking]);

if (component) {
return <ReactRouterRoute {...rest} component={component} />;
Expand All @@ -52,7 +54,7 @@ export const Route = <T extends {}>({
{...rest}
render={(props) => (
<>
<MatchPropagator />
{enableExecutionContextTracking && <MatchPropagator />}
{/* @ts-ignore else condition exists if renderFunction is undefined*/}
{renderFunction(props)}
</>
Expand All @@ -62,7 +64,7 @@ export const Route = <T extends {}>({
}
return (
<ReactRouterRoute {...rest}>
<MatchPropagator />
{enableExecutionContextTracking && <MatchPropagator />}
{children}
</ReactRouterRoute>
);
Expand Down
62 changes: 34 additions & 28 deletions packages/shared-ux/router/impl/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import React, { Children } from 'react';
import { Switch, useRouteMatch } from 'react-router-dom';
import { Routes as ReactRouterRoutes, Route } from 'react-router-dom-v5-compat';
import { Route as LegacyRoute, MatchPropagator } from './route';
import { SharedUXRoutesContext } from './routes_context';

type RouterElementChildren = Array<
React.ReactElement<
Expand All @@ -28,42 +29,47 @@ type RouterElementChildren = Array<

export const Routes = ({
legacySwitch = true,
enableExecutionContextTracking = false,
children,
}: {
legacySwitch?: boolean;
enableExecutionContextTracking?: boolean;
children: React.ReactNode;
}) => {
const match = useRouteMatch();

return legacySwitch ? (
<Switch>{children}</Switch>
<SharedUXRoutesContext.Provider value={{ enableExecutionContextTracking }}>
<Switch>{children}</Switch>
</SharedUXRoutesContext.Provider>
) : (
<ReactRouterRoutes>
{Children.map(children as RouterElementChildren, (child) => {
if (React.isValidElement(child) && child.type === LegacyRoute) {
const path = replace(child?.props.path, match.url + '/', '');
const renderFunction =
typeof child?.props.children === 'function'
? child?.props.children
: child?.props.render;

return (
<Route
path={path}
element={
<>
<MatchPropagator />
{(child?.props?.component && <child.props.component />) ||
(renderFunction && renderFunction()) ||
children}
</>
}
/>
);
} else {
return child;
}
})}
</ReactRouterRoutes>
<SharedUXRoutesContext.Provider value={{ enableExecutionContextTracking }}>
<ReactRouterRoutes>
{Children.map(children as RouterElementChildren, (child) => {
if (React.isValidElement(child) && child.type === LegacyRoute) {
const path = replace(child?.props.path, match.url + '/', '');
const renderFunction =
typeof child?.props.children === 'function'
? child?.props.children
: child?.props.render;
return (
<Route
path={path}
element={
<>
{enableExecutionContextTracking && <MatchPropagator />}
Copy link
Contributor

@Dosant Dosant Dec 17, 2024

Choose a reason for hiding this comment

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

do we need this here? Seems like this will also be called inside <Route/> component

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, but I am not sure how to verify that. Would you mind checking it? If you and your team don't see a use case for it, I can remove it, but I am not sure why it was added in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

I double-checked. Agree that we should keep it here

{(child?.props?.component && <child.props.component />) ||
(renderFunction && renderFunction()) ||
children}
</>
}
/>
);
} else {
return child;
}
})}
</ReactRouterRoutes>
</SharedUXRoutesContext.Provider>
);
};
18 changes: 18 additions & 0 deletions packages/shared-ux/router/impl/routes_context.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { createContext, useContext } from 'react';
import { SharedUXRoutesContextType } from './types';

const defaultContextValue = {};

export const SharedUXRoutesContext = createContext<SharedUXRoutesContextType>(defaultContextValue);

export const useSharedUXRoutesContext = (): SharedUXRoutesContextType =>
useContext(SharedUXRoutesContext as unknown as React.Context<SharedUXRoutesContextType>);
6 changes: 3 additions & 3 deletions packages/shared-ux/router/impl/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export interface SharedUXExecutionContextSetup {
*/
export interface SharedUXExecutionContextSetup {
/** {@link SharedUXExecutionContextSetup} */
executionContext: SharedUXExecutionContextStart;
executionContext?: SharedUXExecutionContextStart;
}

export type KibanaServices = Partial<SharedUXExecutionContextSetup>;
Expand All @@ -63,12 +63,12 @@ const defaultContextValue = {
services: {},
};

export const sharedUXContext =
export const SharedUXContext =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please rename this into SharedUXRouterContext

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in c56ab90

createContext<SharedUXRouterContextValue<KibanaServices>>(defaultContextValue);

export const useKibanaSharedUX = <Extra extends object = {}>(): SharedUXRouterContextValue<
KibanaServices & Extra
> =>
useContext(
sharedUXContext as unknown as React.Context<SharedUXRouterContextValue<KibanaServices & Extra>>
SharedUXContext as unknown as React.Context<SharedUXRouterContextValue<KibanaServices & Extra>>
);
8 changes: 8 additions & 0 deletions packages/shared-ux/router/impl/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,11 @@ export declare interface SharedUXExecutionContext {
/** an inner context spawned from the current context. */
child?: SharedUXExecutionContext;
}

export declare interface SharedUXRoutesContextType {
/**
* This flag is used to disable the default execution context tracking for a specific router.
* Disable this flag in case you have a custom implementation for execution context tracking.
* */
readonly enableExecutionContextTracking?: boolean;
}
2 changes: 1 addition & 1 deletion packages/shared-ux/router/impl/use_execution_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import useDeepCompareEffect from 'react-use/lib/useDeepCompareEffect';
import { SharedUXExecutionContextSetup } from './services';
import type { SharedUXExecutionContextSetup } from './services';
import { SharedUXExecutionContext } from './types';

/**
Expand Down
Loading