Skip to content

Conversation

@vojtechszocs
Copy link
Contributor

@vojtechszocs vojtechszocs commented Nov 13, 2019

This PR adds the necessary changes for consuming Console extensions in a reactive way:

Change summary

  • add flags object to Extension type
  • declare AlwaysOnExtension type as Extension minus flags
  • declare FeatureFlag and ModelDefinition extensions as always-on (anything else is gated)
  • add PluginStore to replace ExtensionRegistry in the future (hook and HOC use this)

Example usage of useExtensions hook:

import {
  useExtensions,
  NavItem,
  Perspective,
  isNavItem,
  isPerspective,
} from '@console/plugin-sdk';

const Example = () => {
  const navItemExtensions = useExtensions<NavItem>(isNavItem);
  const perspectiveExtensions = useExtensions<Perspective>(isPerspective);
  // process extensions and render your component
};

Example usage of withExtensions HOC:

import {
  withExtensions,
  NavItem,
  Perspective,
  isNavItem,
  isPerspective,
} from '@console/plugin-sdk';

const Example = withExtensions<ExampleExtensionProps>({
  navItemExtensions: isNavItem,
  perspectiveExtensions: isPerspective,
})(
  class Example extends React.Component<ExampleOwnProps & ExampleExtensionProps> {
    render() {
      const { navItemExtensions, perspectiveExtensions } = this.props;
      // process extensions and render your component
    }
  },
);

type ExampleExtensionProps = {
  navItemExtensions: NavItem[];
  perspectiveExtensions: Perspective[];
};

@vojtechszocs
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 13, 2019
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/kubevirt Related to kubevirt-plugin component/sdk Related to console-plugin-sdk labels Nov 13, 2019
@vojtechszocs
Copy link
Contributor Author

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 14, 2019
@vojtechszocs
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2019
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, this is needed for code like

-// eslint-disable-next-line no-undef
 if (process.env.NODE_ENV !== 'production') {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, we're using an outdated version of @types/react-redux

$ yarn why @types/react-redux
=> Found "@types/react-redux@6.0.2"

$ yarn why react-redux
=> Found "react-redux@7.1.0"

I've tried to bump it to match react-redux but ran into typing errors, so using @ts-ignore comment for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, can't use flagPending() or FlagsObject from public/reducers/features.ts as it causes Jest to fail on cyclic dependency.

@vojtechszocs vojtechszocs changed the title Gate extensions by feature flags: initial changes Add hook and HOC for consuming Console extensions Nov 15, 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

you can specify state and return value by

Suggested change
const desiredFlags: FlagsObject = useSelector(
const desiredFlags = useSelector<RootState, FlagsObject>(

Copy link
Contributor Author

@vojtechszocs vojtechszocs Nov 19, 2019

Choose a reason for hiding this comment

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

@rawagner Please see #3383 (comment) - we're currently using an outdated version of @types/react-redux so we're missing types for new react-redux APIs like useSelector and shallowEqual.

I'll add type params to useSelector call as suggested.

However, we still need the explicit desiredFlags: FlagsObject type declaration due to missing types - otherwise, this variable would be inferred as any.

Edit: the reason for not updating @types/react-redux to match react-redux version - once we do that, we'll have some type-related errors in existing files - we can fix that in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

getFlagsFromState will run on every store update, maybe we could consider using reselect and comparing if store substate - state.FLAGS changed before getFlagsFromState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From react-redux useSelector docs:

The selector will be run whenever the function component renders. useSelector() will also subscribe to the Redux store, and run your selector whenever an action is dispatched.

With useSelector(), returning a new object every time will always force a re-render by default. If you want to retrieve multiple values from the store, you can:

  • Call useSelector() multiple times, with each call returning a single field value
  • Use Reselect or a similar library to create a memoized selector that returns multiple values in one object, but only returns a new object when one of the values has changed.
  • Use the shallowEqual function from React-Redux as the equalityFn argument to useSelector()

In our useExtensions hook, we're using the 3rd option listed above:

// Compute flags relevant for gating desired extensions
const desiredFlags: FlagsObject = useSelector<RootState, FlagsObject>(
  (state: RootState) => getFlagsFromState(desiredExtensions, state),
  shallowEqual,
);

The useSelector hook already memoizes the object selected from Redux store, based on the second argument (equalityFn) which is === by default, but we're customizing it to shallowEqual comparator.

This is similar to connectToFlags HOC using { areStatePropsEqual: _.isEqual } with the difference that _.isEqual does deep comparison while shallowEqual only compares the object's own properties.

Since we're selecting a simple object { [key: string]: boolean } it doesn't really matter if we use _.isEqual or shallowEqual for comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rawagner But I'm also OK with using _.isEqual instead of shallowEqual (better parity with connectToFlags in terms of comparator), up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that we could use reselect to compare state before running getFlagsFromState. The second argument in useSelector compares result after getFlagsFromState

Copy link
Contributor

Choose a reason for hiding this comment

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

We could compare state.FLAGS with old state.FLAGS as FLAGS are not updated that often

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rawagner Sorry, I misunderstood your comment.

I'll use reselect to memoize on RootState.FLAGS sub-state as suggested.

@vojtechszocs
Copy link
Contributor Author

Added one more commit to handle all NavItem extensions via the new hook & HOC, while also removing getNavItems() from the registry and dropping memoize-one dependency too.

@vojtechszocs vojtechszocs force-pushed the gate-ext-part-1 branch 2 times, most recently from e52760a to 50ede88 Compare January 6, 2020 15:43
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/dev-console Related to dev-console component/knative Related to knative-plugin component/metal3 Related to metal3-plugin and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 6, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020
@spadgett
Copy link
Member

/hold
the merge queue is blocked

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2020
@spadgett
Copy link
Member

/test e2e-gcp-console

@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2020
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 22, 2020
@mareklibra
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jtomasek, mareklibra, rawagner, vojtechszocs

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

The pull request process is described here

Details 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

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/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/network-attachment-definition Related to network-attachment-definition component/noobaa Related to noobaa-storage-plugin component/olm Related to OLM component/sdk Related to console-plugin-sdk lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants