[WIP] Use connectToExtensions HOC for reactive extension consumption

[vojtechszocs]

Fixes: CNV-1772, CNV-1773

👷‍♂️ Work In Progress: needs rebase, ESLint fixups, rework default perspective handling 👷‍♀️

This PR introduces connectToExtensions higher-order component (HOC) used to connect existing components to Console extensions. This is a direct analogy to connect from react-redux, but using Extension[] as the source of truth.

Let's take public/components/dashboards-page/dashboards.tsx as an example.

code before

import { connect } from 'react-redux';

const mapStateToProps = ({ k8s }) => ({
  kindsInFlight: k8s.getIn(['RESOURCES', 'inFlight']),
  k8sModels: k8s.getIn(['RESOURCES', 'models']),
});

const DashboardsPage_: React.FC<DashboardsPageProps> = ({
  match,
  kindsInFlight,
  k8sModels,
}) => {
  // call plugins.registry.getDashboardsCards()
  // call plugins.registry.getDashboardsTabs()
};

export const DashboardsPage = connect(mapStateToProps)(DashboardsPage_);

type DashboardsPageProps = RouteComponentProps & {
  kindsInFlight: boolean;
  k8sModels: ImmutableMap<string, any>;
};

code after

import { connect } from 'react-redux';

import {
  connectToExtensions,
  Extension,
  DashboardsCard,
  DashboardsTab,
  isDashboardsCard,
  isDashboardsTab,
} from '@console/plugin-sdk';

const mapStateToProps = ({ k8s }) => ({
  kindsInFlight: k8s.getIn(['RESOURCES', 'inFlight']),
  k8sModels: k8s.getIn(['RESOURCES', 'models']),
});

const mapExtensionsToProps = (extensions: Extension[]) => ({
  pluginTabs: extensions.filter(isDashboardsTab),
  pluginCards: extensions.filter(isDashboardsCard),
});

const DashboardsPage_: React.FC<DashboardsPageProps> = ({
  match,
  kindsInFlight,
  k8sModels,
  pluginTabs,
  pluginCards,
}) => {
  // no call to plugins.registry
};

export const DashboardsPage = connect(mapStateToProps)(
  connectToExtensions(mapExtensionsToProps)(DashboardsPage_)
);

type DashboardsPageProps = RouteComponentProps & {
  kindsInFlight: boolean;
  k8sModels: ImmutableMap<string, any>;
  pluginCards: DashboardsCard[];
  pluginTabs: DashboardsTab[];
};

Components no longer reference the plugins.registry object directly. Instead, they specify how extensions that are currently in use are mapped to their props via mapExtensionsToProps function.

Extensions in use

An extension is in use (takes effect) when:

• it is an always-on extension (i.e. not gated by any feature flags)

• otherwise, its flags constraints must be satisfied:

  • all required flags are resolved to true
  • all disallowed flags are resolved to false

export type Extension<P = any> = {
  type: string;
  properties: P;
  flags?: Partial<{
    required: string[];
    disallowed: string[];
  }>;
};

export type AlwaysOnExtension<P = any> = Omit<Extension<P>, 'flags'>;

Extensions declared as always-on:

• ModelFeatureFlag - adding new feature flag based on CRD
• ModelDefinition - adding new (static) k8s model definitions

Gating all plugin's extensions

ModelFeatureFlag extension type has a new gateExtensions property, which defaults to true.

When enabled, the corresponding flag is automatically added to flags.required array for all the plugin's extensions (excluding always-on extensions).

The default value of true reflects the general assumption that extensions contributed by plugin X should be gated by flag(s) introduced by plugin X.

Gating specific extensions

Each extension can specify feature flags which are required and/or disallowed in order to put that particular extension into effect.

For example:

{
  type: 'NavItem/Href',
  properties: {
    perspective: 'dev',
    componentProps: { /* stuff */ },
  },
  flags: {
    required: [FLAGS.OPENSHIFT],
  },
},

At runtime, above DevConsole extension's required flags will become:

['OPENSHIFT', 'SHOW_PIPELINE']

due to a FeatureFlag/Model extension adding the SHOW_PIPELINE flag.

---

/cc @spadgett @alecmerdler @christianvogt @jelkosz @rawagner

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vojtechszocs

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:

• frontend/OWNERS [vojtechszocs]

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

[vojtechszocs]

/hold

[rawagner]

great work @vojtechszocs !

I wonder if it would be beneficial to store extensions in redux store and then the example you posted would look like

import { connect } from 'react-redux';

import {
  connectToExtensions,
  Extension,
  DashboardsCard,
  DashboardsTab,
  isDashboardsCard,
  isDashboardsTab,
} from '@console/plugin-sdk';

const mapStateToProps = ({ k8s, extensions }) => ({
  kindsInFlight: k8s.getIn(['RESOURCES', 'inFlight']),
  k8sModels: k8s.getIn(['RESOURCES', 'models']),
  pluginTabs: extensions.filter(isDashboardsTab),
  pluginCards: extensions.filter(isDashboardsCard),
});

const DashboardsPage_: React.FC<DashboardsPageProps> = ({
  match,
  kindsInFlight,
  k8sModels,
  pluginTabs,
  pluginCards,
}) => {
  // no call to plugins.registry
};

export const DashboardsPage = connect(mapStateToProps)(DashboardsPage_);

type DashboardsPageProps = RouteComponentProps & {
  kindsInFlight: boolean;
  k8sModels: ImmutableMap<string, any>;
  pluginCards: DashboardsCard[];
  pluginTabs: DashboardsTab[];
};

is there any downside to this approach ?

[vojtechszocs]

Rebased on latest master and squashed some commits.

[vojtechszocs]

Fixed all ESLint issues.

[vojtechszocs]

Thanks @rawagner for your review! 😃

I wonder if it would be beneficial to store extensions in redux store

connectToExtensions HOC creator is actually doing two things:

• compute extensions currently in use, i.e. pluginStore.getExtensionsInUse(flags)
• call mapExtensionsToProps and pass those additional props to wrapped Component

This is similar to existing HOC creators like connectToFlags.

Removing connectToExtensions in favor of using standard react-redux connect directly, we shift the responsibility of performing related computations to consumers:

import {
  getExtensionsInUse,
  isDashboardsTab,
  isDashboardsCard,
} from '@console/plugin-sdk';

// in this example, "extensions" refers to *all* extensions stored in Redux
const mapStateToProps = ({ k8s, extensions }) => ({
  kindsInFlight: k8s.getIn(['RESOURCES', 'inFlight']),
  k8sModels: k8s.getIn(['RESOURCES', 'models']),
  pluginTabs: getExtensionsInUse(extensions.filter(isDashboardsTab)),
  pluginCards: getExtensionsInUse(extensions.filter(isDashboardsCard)),
});

but this could be optimized by storing extensionsInUse in Redux as derived data, updated whenever extensions (source of truth) is changed.

const mapStateToProps = ({ k8s, extensionsInUse }) => ({
  kindsInFlight: k8s.getIn(['RESOURCES', 'inFlight']),
  k8sModels: k8s.getIn(['RESOURCES', 'models']),
  pluginTabs: extensionsInUse.filter(isDashboardsTab),
  pluginCards: extensionsInUse.filter(isDashboardsCard),
});

In terms of a reducer function (i.e. public/reducers/extensions.ts), the initialization of related sub-state (first argument of reducer) would depend on code like:

const activePlugins = (process.env.NODE_ENV !== 'test')
  ? require('@console/active-plugins').default as ActivePlugin[]
  : [];

breaking the constraint of reducer being a pure function. But we could work around this by passing pre-initialized extensions to createStore function, shifting the impurity to code outside the reducer:

// the second argument, "{}", is the initial Redux state
const store = createStore(reducers, {}, composeEnhancers(applyMiddleware(thunk)));

TL;DR

I'd favor more specific HOC creators like connectToFlags and connectToExtensions over using the generic connect, since the latter forces us to

• either represent the computed data as derived data stored & managed in Redux
  • adds complexity to reducer functions
• or shift the responsibility of computing data to consumers
  • spreads complexity over the codebase

As for how to store Console extensions, I'm fine with having them managed via Redux state too.

I'm also curious what other people think about this.

[vojtechszocs]

@christianvogt @spadgett @jelkosz

The only remaining issue before removing the WIP label is

• rework default perspective handling (part of 1fea925)

Reducer in public/reducers/ui.ts calls plugins.registry.getPerspectives() as part of the sub-state (state: UIState) initialization. This call is sync, imposing the assumption that extensions of type Perspective are available as early as during Console startup.

However, by design, only ModelFeatureFlag and ModelDefinition extensions are always-on. With the current code in packages/dev-console/src/plugin.tsx, the "Developer" perspective is implicitly gated by the SHOW_PIPELINE flag.

Note: the default "Administrator" perspective (packages/console-app/src/plugin.tsx) isn't gated by any flags, and therefore in use right upon Console startup.

So what's the best way to solve this problem?

• postpone Console startup until flags used to gate perspective extensions are resolved (UX impact)
• remove default perspective handling
• any other solution?

[vojtechszocs]

FYI @mareklibra @jtomasek @knowncitizen @honza @cloudbehl @joshuawilson @rohitkrai03

[knowncitizen]

Thanks for the HU on this @vojtechszocs

[openshift-ci-robot]

@vojtechszocs: This pull request references a valid Bugzilla bug. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

[WIP] Bug 1738292: Use connectToExtensions HOC for reactive extension consumption

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[christianvogt]

@vojtechszocs I dislike the behavior of gateExtensions. It's too magical. I would prefer to be explicit. You've introduced a flags prop to the extension, let the author be explicit and supply the flag even if it seems redundant.
If the code is too redundant, provide a utility to flag all supplied extensions to a specified flag.
I think this would remove the notion of always on extensions.

[vojtechszocs]

Note to self (cc @christianvogt): remove ModelFeatureFlag.gateExtensions property, let people be explicit which flags are required per each extension.

[openshift-ci-robot]

@vojtechszocs: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

[WIP] Use connectToExtensions HOC for reactive extension consumption

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vojtechszocs]

/bugzilla refresh

[openshift-ci-robot]

@vojtechszocs: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vojtechszocs]

Original action items

• rework default perspective handling as per [WIP] Use connectToExtensions HOC for reactive extension consumption #2269 (comment)
• remove ModelFeatureFlag.gateExtensions property as per [WIP] Use connectToExtensions HOC for reactive extension consumption #2269 (comment)
• add useExtensions hook to complement connectToExtensions HOC in functional components

Additional action items

• rebase on Bug 1738292: Gate dashboard extensions by feature flags #2446 & remove any extension-specific gating properties like required: string
• address TODO items where the react-hooks/exhaustive-deps ESLint rule is disabled
• avoid using the useEffectDeepCompare hook (or similar custom comparison helper)

Cleanup items

• address TODO(vojtech) comments related to code removal

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[openshift-ci-robot]

@vojtechszocs: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/frontend	fad3aaa	link	/test frontend
ci/prow/e2e-aws-console	fad3aaa	link	/test e2e-aws-console
ci/prow/images	fad3aaa	link	/test images
ci/prow/e2e-aws-console-olm	fad3aaa	link	/test e2e-aws-console-olm
ci/prow/backend	fad3aaa	link	/test backend
ci/prow/e2e-aws	fad3aaa	link	/test e2e-aws
ci/prow/e2e-gcp	fad3aaa	link	/test e2e-gcp
ci/prow/e2e-gcp-console	fad3aaa	link	/test e2e-gcp-console
ci/prow/analyze	fad3aaa	link	/test analyze
ci/prow/kubevirt-plugin	fad3aaa	link	/test kubevirt-plugin

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[openshift-bot]

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

[openshift-ci-robot]

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.