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

adding console.pages extension types to dynamic-plugin-sdk #8818

Merged

Conversation

glekner
Copy link
Contributor

@glekner glekner commented Apr 28, 2021

No description provided.

@openshift-ci-robot openshift-ci-robot added the component/sdk Related to console-plugin-sdk label Apr 28, 2021
@glekner
Copy link
Contributor Author

glekner commented Apr 28, 2021

/retest

@@ -1,6 +1,25 @@
import { RouteComponentProps } from 'react-router';
import { Extension, ExtensionDeclaration, CodeRef } from '../types';

type ResourcePage = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type ResourcePage = {
type ResourcePageProperties = {

@@ -1,6 +1,25 @@
import { RouteComponentProps } from 'react-router';
import { Extension, ExtensionDeclaration, CodeRef } from '../types';

type ResourcePage = {
model: {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have multiple declarations of type literal

model: {
  group: string;
  version: string;
  kind: string;
};

as well as this one

model: {
  group: string;
  version?: string;
  kind?: string;
};

I think it's best to create a helper type in packages/console-dynamic-plugin-sdk/src/api/common-types.ts to represent a k8s model/resource and use that one for better consistency.

This will also keep the generated JSON schema simpler (references to type definitions, instead of inlining those definitions everywhere they're used).

'console.page/route',
{
/** The React component to load for the page. */
component: CodeRef<React.Component<RouteComponentProps>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

React.Component refers to class components only. React.FC refers to function components only.

Please use React.ComponentType here, as well as in StandaloneRoutePage extension declaration above.

type ComponentType<P = {}> = ComponentClass<P> | FunctionComponent<P>;

In general, all extension declarations should use React.ComponentType when referring to a React component.

export type RoutePage = ExtensionDeclaration<
'console.page/route',
{
/** The React component to load for the page. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** The React component to load for the page. */
/** The component to be rendered when the route matches. */

{
/** The React component to load for the page. */
component: CodeRef<React.Component<RouteComponentProps>>;
/** The perspective ID to which this item belongs to. If not specified, contributes to all perspectives. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** The perspective ID to which this item belongs to. If not specified, contributes to all perspectives. */
/** The perspective to which this page belongs to. If not specified, contributes to all perspectives. */

/** When true, will only match if the path matches the `location.pathname` exactly. */
exact?: boolean;
/** When true, a path that has a trailing slash will only match a location.pathname with a trailing slash. This has no effect when there are additional URL segments in the location.pathname. */
strict?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please go over all usages of the current Page/Route extension and see if we really need those additional properties, like strict and sensitive.

}
>;

/** Adds new resource list to Console router. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Adds new resource list to Console router. */
/** Adds new resource list page to Console router. */

/** Adds new resource list to Console router. */
export type ResourceListPage = ExtensionDeclaration<'console.page/resource/list', ResourcePage>;

/** Adds new details page to Console router. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Adds new details page to Console router. */
/** Adds new resource details page to Console router. */

/** When true, will only match if the path matches the `location.pathname` exactly. */
exact?: boolean;
/** When true, a path that has a trailing slash will only match a location.pathname with a trailing slash. This has no effect when there are additional URL segments in the location.pathname. */
strict?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please go over all usages of the current Page/Resource/Tab extension and see if we really need those additional properties, like strict and sensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could be useful for later usages?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can always add more properties later, I'd recommend to keep the API surface minimal 😃

@vojtechszocs
Copy link
Contributor

We should avoid duplicating common properties, like

/** The React component to load for the page. */
component: CodeRef<React.ComponentType<RouteComponentProps>>;

by creating meaningful helper types, e.g. in packages/console-dynamic-plugin-sdk/src/extensions/navigation.ts we have NavItemProperties type which is used in multiple ways, including

Omit<NavItemProperties, 'foo'> // remove properties
NavItemProperties & { bar: string } // add properties
Omit<NavItemProperties, 'foo'> & { bar: string } // combination of above

@glekner glekner force-pushed the dp-types-route-page branch 2 times, most recently from afab61d to 05c325d Compare April 29, 2021 13:25
@glekner glekner force-pushed the dp-types-route-page branch 3 times, most recently from b0cd866 to eb4fc9d Compare April 29, 2021 16:56
/** Adds new resource list page to Console router. */
export type ResourceListPage = ExtensionDeclaration<
'console.page/resource/list',
ResourcePageProperties & { model: ExtensionK8sModel }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ResourcePageProperties & { model: ExtensionK8sModel }
ResourcePageProperties & {}

/** Adds new resource details page to Console router. */
export type ResourceDetailsPage = ExtensionDeclaration<
'console.page/resource/details',
ResourcePageProperties & { model: ExtensionK8sModel }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ResourcePageProperties & { model: ExtensionK8sModel }
ResourcePageProperties & {}

/** Adds new resource tab page to Console router. */
export type ResourceTabPage = ExtensionDeclaration<
'console.page/resource/tab',
ResourcePageProperties & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ResourcePageProperties & {
Omit<ResourcePageProperties, 'component'> & {

/** Adds new standalone page (rendered outside the common page layout) to Console router. */
export type StandaloneRoutePage = ExtensionDeclaration<
'console.page/route/standalone',
Omit<RoutePageProperties, 'perspective' | 'strict' | 'sensitive'>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Omit<RoutePageProperties, 'perspective' | 'strict' | 'sensitive'>
Omit<RoutePageProperties, 'perspective'>

export const isResourceTabPage = (e: Extension): e is ResourceTabPage =>
e.type === 'console.page/resource/tab';

export type ResourcePage = ResourceListPage | ResourceDetailsPage;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest removing this union type here.

In public/components/resource-pages.ts we can add

type ResourcePage = ResourceListPage | ResourceDetailsPage;

since the fact that ^^ code handles both resource page extensions is not really relevant to extension definitions.

@rawagner
Copy link
Contributor

rawagner commented May 3, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, rawagner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2021
@openshift-merge-robot openshift-merge-robot merged commit 6561570 into openshift:master May 3, 2021
@spadgett spadgett added this to the v4.8 milestone May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/sdk Related to console-plugin-sdk lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants