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

Cleanup spaces plugin #91976

Merged
merged 27 commits into from
Mar 1, 2021
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
fad2f42
Convert relative imports to absolute imports
jportner Feb 18, 2021
388206d
Convert more relative imports to absolute imports
jportner Feb 18, 2021
dc49b70
Temporarily remove `import type` directive
jportner Feb 18, 2021
25a9066
Sort imports
jportner Feb 19, 2021
97cc9bb
Apply `import type` directive
jportner Feb 19, 2021
4ad077f
Add lazy loading of reusable Spaces UI components
jportner Feb 19, 2021
f022c4c
Reduce bundle size
jportner Feb 22, 2021
59a921c
Fix jest test error
jportner Feb 22, 2021
6284843
Refactor CTS flyout
jportner Feb 22, 2021
0872427
Move summarizeCopyResult
jportner Feb 22, 2021
9341c0c
Split CTS flyout into a separate chunk
jportner Feb 22, 2021
0080228
Lazy load NavControlPopover
jportner Feb 22, 2021
f3126fd
Refactor SpaceAvatar
jportner Feb 22, 2021
9b6cf54
Lazy load SpaceAvatar
jportner Feb 22, 2021
c922c54
Remove dependency on @kbn/std package
jportner Feb 22, 2021
b8d93b0
Change CTS flyout to use SpacesContext
jportner Feb 22, 2021
6242d1a
Merge branch 'master' into pr/jportner/91976
jportner Feb 25, 2021
a21ca9a
Abstract out lazy-loading from reusable components
jportner Feb 25, 2021
032b5cb
Use error boundaries when lazy-loading components
jportner Feb 25, 2021
bebf3b2
Rename `getSpacesContext` to `getSpacesContextProvider`
jportner Feb 25, 2021
b792353
Fix type check for unit test
jportner Feb 25, 2021
181ddcb
Fix i18n error
jportner Feb 25, 2021
2612620
Remove unused parameter
jportner Feb 25, 2021
1b6554b
Merge branch 'master' into pr/jportner/91976
jportner Feb 25, 2021
1b6c5e5
PR review feedback
jportner Feb 25, 2021
38fb012
Fix jest unit tests
jportner Feb 25, 2021
99ab44c
Merge branch 'master' into cleanup-spaces-plugin
kibanamachine Mar 1, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,20 @@ import { Query } from '@elastic/eui';
import { parse } from 'query-string';
import { i18n } from '@kbn/i18n';
import { CoreStart, ChromeBreadcrumb } from 'src/core/public';
import type {
SpacesAvailableStartContract,
SpacesContextProps,
} from 'src/plugins/spaces_oss/public';
import { DataPublicPluginStart } from '../../../data/public';
import { SavedObjectsTaggingApi } from '../../../saved_objects_tagging_oss/public';
import type { SpacesAvailableStartContract } from '../../../spaces_oss/public';
import {
ISavedObjectsManagementServiceRegistry,
SavedObjectsManagementActionServiceStart,
SavedObjectsManagementColumnServiceStart,
} from '../services';
import { SavedObjectsTable } from './objects_table';

const EmptyFunctionComponent: React.FC = ({ children }) => <>{children}</>;
const getEmptyFunctionComponent: React.FC<SpacesContextProps> = ({ children }) => <>{children}</>;

const SavedObjectsTablePage = ({
coreStart,
Expand Down Expand Up @@ -71,7 +74,8 @@ const SavedObjectsTablePage = ({
}, [setBreadcrumbs]);

const ContextWrapper = useMemo(
() => spacesApi?.ui.components.SpacesContext || EmptyFunctionComponent,
() =>
spacesApi ? spacesApi.ui.components.getSpacesContextProvider : getEmptyFunctionComponent,
[spacesApi]
);

Expand Down
9 changes: 5 additions & 4 deletions src/plugins/spaces_oss/public/api.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@ type SpacesApiUiComponentMock = jest.Mocked<SpacesApiUiComponent>;

const createApiUiComponentsMock = () => {
const mock: SpacesApiUiComponentMock = {
SpacesContext: jest.fn(),
ShareToSpaceFlyout: jest.fn(),
SpaceList: jest.fn(),
LegacyUrlConflict: jest.fn(),
getSpacesContextProvider: jest.fn(),
getShareToSpaceFlyout: jest.fn(),
getSpaceList: jest.fn(),
getLegacyUrlConflict: jest.fn(),
getSpaceAvatar: jest.fn(),
};

return mock;
Expand Down
38 changes: 32 additions & 6 deletions src/plugins/spaces_oss/public/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { Observable } from 'rxjs';
import type { FunctionComponent } from 'react';
import type { ReactElement } from 'react';
import { Space } from '../common';

/**
Expand All @@ -22,12 +22,19 @@ export interface SpacesApi {
ui: SpacesApiUi;
}

/**
* Function that returns a promise for a lazy-loadable component.
*
* @public
*/
export type LazyComponentFn<T> = (props: T) => ReactElement;

/**
* @public
*/
export interface SpacesApiUi {
/**
* {@link SpacesApiUiComponent | React components} to support the spaces feature.
* Lazy-loadable {@link SpacesApiUiComponent | React components} to support the spaces feature.
*/
components: SpacesApiUiComponent;
/**
Expand Down Expand Up @@ -62,13 +69,13 @@ export interface SpacesApiUiComponent {
/**
* Provides a context that is required to render some Spaces components.
*/
SpacesContext: FunctionComponent<SpacesContextProps>;
getSpacesContextProvider: LazyComponentFn<SpacesContextProps>;
/**
* Displays a flyout to edit the spaces that an object is shared to.
*
* Note: must be rendered inside of a SpacesContext.
*/
ShareToSpaceFlyout: FunctionComponent<ShareToSpaceFlyoutProps>;
getShareToSpaceFlyout: LazyComponentFn<ShareToSpaceFlyoutProps>;
/**
* Displays a corresponding list of spaces for a given list of saved object namespaces. It shows up to five spaces (and an indicator for
* any number of spaces that the user is not authorized to see) by default. If more than five named spaces would be displayed, the extras
Expand All @@ -77,7 +84,7 @@ export interface SpacesApiUiComponent {
*
* Note: must be rendered inside of a SpacesContext.
*/
SpaceList: FunctionComponent<SpaceListProps>;
getSpaceList: LazyComponentFn<SpaceListProps>;
/**
* Displays a callout that needs to be used if a call to `SavedObjectsClient.resolve()` results in an `"conflict"` outcome, which
* indicates that the user has loaded the page which is associated directly with one object (A), *and* with a legacy URL that points to a
Expand All @@ -95,7 +102,11 @@ export interface SpacesApiUiComponent {
*
* New URL path: `#/workpad/workpad-e08b9bdb-ec14-4339-94c4-063bddfd610e/page/1`
*/
LegacyUrlConflict: FunctionComponent<LegacyUrlConflictProps>;
getLegacyUrlConflict: LazyComponentFn<LegacyUrlConflictProps>;
/**
* Displays an avatar for the given space.
*/
getSpaceAvatar: LazyComponentFn<SpaceAvatarProps>;
}

/**
Expand Down Expand Up @@ -251,3 +262,18 @@ export interface LegacyUrlConflictProps {
*/
otherObjectPath: string;
}

/**
* @public
*/
export interface SpaceAvatarProps {
space: Partial<Space>;
size?: 's' | 'm' | 'l' | 'xl';
className?: string;
/**
* When enabled, allows EUI to provide an aria-label for this component, which is announced on screen readers.
*
* Default value is true.
*/
announceSpaceName?: boolean;
}
2 changes: 2 additions & 0 deletions src/plugins/spaces_oss/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export {
} from './types';

export {
LazyComponentFn,
SpacesApi,
SpacesApiUi,
SpacesApiUiComponent,
Expand All @@ -24,6 +25,7 @@ export {
ShareToSpaceSavedObjectTarget,
SpaceListProps,
LegacyUrlConflictProps,
SpaceAvatarProps,
} from './api';

export const plugin = () => new SpacesOssPlugin();
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import React, { FC, useState } from 'react';
import React, { FC, useMemo, useState } from 'react';

import { EuiButtonEmpty } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
Expand Down Expand Up @@ -66,7 +66,11 @@ export const JobSpacesList: FC<Props> = ({ spacesApi, spaceIds, jobId, jobType,
});
}

const { SpaceList, ShareToSpaceFlyout } = spacesApi.ui.components;
const LazySpaceList = useMemo(() => spacesApi.ui.components.getSpaceList, [spacesApi]);
const LazyShareToSpaceFlyout = useMemo(() => spacesApi.ui.components.getShareToSpaceFlyout, [
spacesApi,
]);

const shareToSpaceFlyoutProps: ShareToSpaceFlyoutProps = {
savedObjectTarget: {
type: ML_SAVED_OBJECT_TYPE,
Expand All @@ -83,9 +87,9 @@ export const JobSpacesList: FC<Props> = ({ spacesApi, spaceIds, jobId, jobType,
return (
<>
<EuiButtonEmpty onClick={() => setShowFlyout(true)} style={{ height: 'auto' }}>
<SpaceList namespaces={spaceIds} displayLimit={0} behaviorContext="outside-space" />
<LazySpaceList namespaces={spaceIds} displayLimit={0} behaviorContext="outside-space" />
</EuiButtonEmpty>
{showFlyout && <ShareToSpaceFlyout {...shareToSpaceFlyoutProps} />}
{showFlyout && <LazyShareToSpaceFlyout {...shareToSpaceFlyoutProps} />}
</>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
EuiTabbedContentTab,
} from '@elastic/eui';

import type { SpacesContextProps } from 'src/plugins/spaces_oss/public';
import { PLUGIN_ID } from '../../../../../../common/constants/app';
import { ManagementAppMountParams } from '../../../../../../../../../src/plugins/management/public/';

Expand Down Expand Up @@ -67,7 +68,7 @@ function usePageState<T extends ListingPageUrlState>(
return [pageState, updateState];
}

const EmptyFunctionComponent: React.FC = ({ children }) => <>{children}</>;
const getEmptyFunctionComponent: React.FC<SpacesContextProps> = ({ children }) => <>{children}</>;

function useTabs(isMlEnabledInSpace: boolean, spacesApi: SpacesPluginStart | undefined): Tab[] {
const [adPageState, updateAdPageState] = usePageState(getDefaultAnomalyDetectionJobsListState());
Expand Down Expand Up @@ -147,6 +148,12 @@ export const JobsListPage: FC<{
check();
}, []);

const ContextWrapper = useMemo(
() =>
spacesApi ? spacesApi.ui.components.getSpacesContextProvider : getEmptyFunctionComponent,
[spacesApi]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably don't have enough context (lol, see what I did there?) so might be better to catch up tomorrow but it would be nice to be a bit clearer here in regards to naming.

getSpacesContext to me sounds like we're getting a React Context object for spaces. That's usually a static object which contains a Provider and a Consumer component. In this case however, we are wrapping it in React.lazy to create a lazy loaded component so it's just the Provider component by the looks of it? In that case it might be clearer to call the method getSpacesProvider and the return value SpacesProvider.

Having said that, Context (returned by createContext) is usually a static object in React so we could just make that available through the spaces plugin contract directly rather than having these getter functions and having to memoize them.

Anyways, let's catch up tomorrow, just adding this as a reminder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I share the same concerns. React context providers are usually static, and the value they provide mutates. I'm not sure to understand why we need an lazy accessor around a context provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be nice to be a bit clearer here in regards to naming.

I'm open to naming changes!

getSpacesContext to me sounds like we're getting a React Context object for spaces. That's usually a static object which contains a Provider and a Consumer component. In this case however, we are wrapping it in React.lazy to create a lazy loaded component so it's just the Provider component by the looks of it?

Sort of. It's a "wrapper" component that includes the context provider. We need this because when this wrapper is mounted, it makes a series of API calls and caches the results, then makes those available to its children via the context provider.

The function to obtain this wrapper within the Spaces plugin is called getSpacesContextWrapper, but the one that is exposed to consumers in the public contract is called getSpacesContext. We could rename the latter to something like getSpacesContextProviderWrapper but I don't know what value that really adds for consumers, it's an implementation detail IMO. They don't need to consume the context provider, they just need to know that they have to render the flyout, space list, etc. as a descendant of this wrapper.

Having said that, Context (returned by createContext) is usually a static object in React so we could just make that available through the spaces plugin contract directly rather than having these getter functions and having to memoize them.

I don't think so, we need the wrapper.

I'm not sure to understand why we need an lazy accessor around a context provider?

Because it's not just a context provider, it's the wrapper that also fetches data. Also, to flip that question around: why not provide a lazy accessor? All of the other components have lazy accessors. It seems this approach is more consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Caught up with Joe and I can't think of a good alternative to avoid the architectural complexity with the approach here.

To fulfil the brief, we need a way to make the spaces state available globally, so that we can provide drop-in components for developers that share that state. However, we're not able to render the context provider at the root level of the application ourselves (since spaces is a plugin) and we can't provide static exports (due to the restriction imposed by the platform), hence the indirection using dynamically created and memoized context providers, that require developers to consume them using the spaces plugin contract rather than static imports like EUI.

I'm guessing this is a bit of an edge case and other plugin developers don't have the same requirements to share core functionality like spaces does. However, I would argue that certain things like i18n, routing, session information, authenticated user information and in this particular case spaces are global concerns that should be available throughout the application. Usually these are managed at the root level in React applications so that any child component has access via hooks and context but this isn't really possible with the way we've currently modelled spaces as a plugin.

Happy to leave this as is for now but I am slightly worried about the complexity and maintainability of the current solution.


if (initialized === false) {
return null;
}
Expand Down Expand Up @@ -185,8 +192,6 @@ export const JobsListPage: FC<{
return <AccessDeniedPage />;
}

const ContextWrapper = spacesApi?.ui.components.SpacesContext || EmptyFunctionComponent;

return (
<RedirectAppLinks application={coreStart.application}>
<I18nContext>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
NotificationsStart,
} from 'src/core/public';
import type { DocLinksStart, ScopedHistory } from 'kibana/public';
import type { SpacesApiUi } from 'src/plugins/spaces_oss/public';
import { FeaturesPluginStart } from '../../../../../features/public';
import { KibanaFeature } from '../../../../../features/common';
import { IndexPatternsContract } from '../../../../../../../src/plugins/data/public';
Expand Down Expand Up @@ -84,6 +85,7 @@ interface Props {
notifications: NotificationsStart;
fatalErrors: FatalErrorsSetup;
history: ScopedHistory;
spacesApiUi?: SpacesApiUi;
}

function useRunAsUsers(
Expand Down Expand Up @@ -289,6 +291,7 @@ export const EditRolePage: FunctionComponent<Props> = ({
uiCapabilities,
notifications,
history,
spacesApiUi,
}) => {
const backToRoleList = useCallback(() => history.push('/'), [history]);

Expand Down Expand Up @@ -447,6 +450,7 @@ export const EditRolePage: FunctionComponent<Props> = ({
role={role}
onChange={onRoleChange}
validator={validator}
spacesApiUi={spacesApiUi}
/>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import React, { Component } from 'react';
import { Capabilities } from 'src/core/public';
import type { SpacesApiUi } from 'src/plugins/spaces_oss/public';
import { Space } from '../../../../../../../spaces/public';
import { Role } from '../../../../../../common/model';
import { RoleValidator } from '../../validate_role';
Expand All @@ -26,6 +27,7 @@ interface Props {
kibanaPrivileges: KibanaPrivileges;
onChange: (role: Role) => void;
validator: RoleValidator;
spacesApiUi?: SpacesApiUi;
}

export class KibanaPrivilegesRegion extends Component<Props, {}> {
Expand All @@ -48,6 +50,7 @@ export class KibanaPrivilegesRegion extends Component<Props, {}> {
onChange,
editable,
validator,
spacesApiUi,
} = this.props;

if (role._transform_error && role._transform_error.includes('kibana')) {
Expand All @@ -65,6 +68,7 @@ export class KibanaPrivilegesRegion extends Component<Props, {}> {
editable={editable}
canCustomizeSubFeaturePrivileges={canCustomizeSubFeaturePrivileges}
validator={validator}
spacesApiUi={spacesApiUi!}
/>
);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,15 @@

import React from 'react';
import { mountWithIntl } from '@kbn/test/jest';
import { spacesManagerMock } from '../../../../../../../../spaces/public/spaces_manager/mocks';
import { getUiApi } from '../../../../../../../../spaces/public/ui_api';
import { createKibanaPrivileges } from '../../../../__fixtures__/kibana_privileges';
import { kibanaFeatures } from '../../../../__fixtures__/kibana_features';
import { RoleKibanaPrivilege } from '../../../../../../../common/model';
import { PrivilegeSummary } from '.';
import { findTestSubject } from '@kbn/test/jest';
import { PrivilegeSummaryTable } from './privilege_summary_table';
import { coreMock } from 'src/core/public/mocks';

const createRole = (roleKibanaPrivileges: RoleKibanaPrivilege[]) => ({
name: 'some-role',
Expand All @@ -31,6 +34,9 @@ const spaces = [
disabledFeatures: [],
},
];
const spacesManager = spacesManagerMock.create();
const { getStartServices } = coreMock.createSetup();
const spacesApiUi = getUiApi({ spacesManager, getStartServices });

describe('PrivilegeSummary', () => {
it('initially renders a button', () => {
Expand All @@ -50,6 +56,7 @@ describe('PrivilegeSummary', () => {
kibanaPrivileges={kibanaPrivileges}
role={role}
canCustomizeSubFeaturePrivileges={true}
spacesApiUi={spacesApiUi}
/>
);

Expand All @@ -74,6 +81,7 @@ describe('PrivilegeSummary', () => {
kibanaPrivileges={kibanaPrivileges}
role={role}
canCustomizeSubFeaturePrivileges={true}
spacesApiUi={spacesApiUi}
/>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { FormattedMessage } from '@kbn/i18n/react';
import { EuiButtonEmpty, EuiOverlayMask, EuiButton } from '@elastic/eui';
import { EuiFlyout } from '@elastic/eui';
import { EuiFlyoutHeader, EuiTitle, EuiFlyoutBody, EuiFlyoutFooter } from '@elastic/eui';
import type { SpacesApiUi } from 'src/plugins/spaces_oss/public';
import { Space } from '../../../../../../../../spaces/public';
import { Role } from '../../../../../../../common/model';
import { PrivilegeSummaryTable } from './privilege_summary_table';
Expand All @@ -20,6 +21,7 @@ interface Props {
spaces: Space[];
kibanaPrivileges: KibanaPrivileges;
canCustomizeSubFeaturePrivileges: boolean;
spacesApiUi: SpacesApiUi;
}
export const PrivilegeSummary = (props: Props) => {
const [isOpen, setIsOpen] = useState(false);
Expand Down Expand Up @@ -54,6 +56,7 @@ export const PrivilegeSummary = (props: Props) => {
spaces={props.spaces}
kibanaPrivileges={props.kibanaPrivileges}
canCustomizeSubFeaturePrivileges={props.canCustomizeSubFeaturePrivileges}
spacesApiUi={props.spacesApiUi}
/>
</EuiFlyoutBody>
<EuiFlyoutFooter>
Expand Down
Loading