Improve Console extension React hook & HOC #4211

vojtechszocs · 2020-02-05T17:55:10Z

This PR includes the following improvements:

pick up changes from Add hook and HOC for consuming Console extensions #3383 that got lost during rebase ☕ (withExtensions parameter change)
fix useExtensions not memoizing its result properly (gatingFlags was always recomputed)

useExtensions result value can now be used as a "stable" dependency of other hooks:

React.useMemo
React.useEffect (this one also throws if one of its dependencies changes on every render)
etc.

cc @christianvogt @spadgett @benjaminapetersen @rawagner

vojtechszocs · 2020-02-05T18:17:36Z

Reselect (createSelectorCreator) + React-Redux (useSelector) approach to memoizing a value computed from Redux state is based on @rawagner's code (feels a bit complex but works perfectly):

console/frontend/public/components/utils/k8s-watch-hook.ts

Line 24 in 67bc435

const k8sModelSelectorCreator = React.useMemo(

vojtechszocs · 2020-02-06T13:09:38Z

/retest

christianvogt · 2020-03-05T16:19:23Z

frontend/public/components/nav/nav-header.tsx

@@ -104,7 +104,7 @@ const NavHeader_: React.FC<NavHeaderProps & StateProps> = ({

 const mapStateToProps = (state: RootState): StateProps => ({
  activePerspective: getActivePerspective(state),
-  flags: stateToFlagsObject(state),
+  flags: state[featureReducerName].toObject(),


Would be still useful to provide a reusable selector here.

christianvogt · 2020-03-05T16:20:24Z

frontend/public/reducers/features.ts

+export const stateToFlagsObject = (state: FeatureState, desiredFlags: string[]): FlagsObject =>
+  desiredFlags.reduce((allFlags, f) => ({ ...allFlags, [f]: state.get(f) }), {} as FlagsObject);


support desiredFlags as option so we can continue to use this selector as previously implemented.
If no desiredFlags supplied, fallback to all flags.

We can do that, but we should also differentiate between "empty flags" (empty array) vs. "all flags" (falsy value) cases.

I'll make desiredFlags optional and fallback to null which would allow us to reuse it like before 😃

I didn't realize this function was using FeatureState.
This is change request was related to #4211 (comment) where it would have been nice to keep a selector.

christianvogt · 2020-03-05T16:24:06Z

frontend/packages/console-plugin-sdk/src/useExtensions.ts

+  const gatingFlagSelectorCreator = React.useMemo(
+    () =>
+      createSelectorCreator(
+        defaultMemoize as any,


can we avoid the use of any here?

So far, neither @rawagner nor I found a way to avoid it, see also #3443 (comment).

TypeScript "test" for Reselect contains the following code (link):

createSelectorCreator(defaultMemoize, <T>(a: T, b: T, index: number) => { if (index === 0) return a === b; return a.toString() === b.toString(); });

So I've tried to adapt it to our code.

My 1st iteration - doesn't work, since T in <T>(a: T, b: T) => boolean is basically any object:

createSelectorCreator( defaultMemoize, <FeatureState>(prevFeatureState: FeatureState, nextFeatureState: FeatureState) => // TS error: property 'get' does not exist on 'FeatureState' gatingFlagNames.every((f) => prevFeatureState.get(f) === nextFeatureState.get(f)), )

My 2nd iteration - doesn't work as well:

createSelectorCreator( defaultMemoize, <T extends FeatureState>(prevFeatureState: T, nextFeatureState: T) => // TS error: 'T' is not assignable to 'FeatureState' gatingFlagNames.every((f) => prevFeatureState.get(f) === nextFeatureState.get(f)), )

It seems that Reselect typings for createSelectorCreator API aren't robust enough.

Maybe a bug in the typing of defaultMemoize.
No problem keep it.

vojtechszocs · 2020-03-05T17:38:50Z

frontend/packages/console-plugin-sdk/src/useExtensions.ts

+  const gatingFlagSelectorCreator = React.useMemo(
+    () =>
+      createSelectorCreator(
+        defaultMemoize as any,


So far, neither @rawagner nor I found a way to avoid it, see also #3443 (comment).

TypeScript "test" for Reselect contains the following code (link):

createSelectorCreator(defaultMemoize, <T>(a: T, b: T, index: number) => { if (index === 0) return a === b; return a.toString() === b.toString(); });

So I've tried to adapt it to our code.

My 1st iteration - doesn't work, since T in <T>(a: T, b: T) => boolean is basically any object:

createSelectorCreator( defaultMemoize, <FeatureState>(prevFeatureState: FeatureState, nextFeatureState: FeatureState) => // TS error: property 'get' does not exist on 'FeatureState' gatingFlagNames.every((f) => prevFeatureState.get(f) === nextFeatureState.get(f)), )

My 2nd iteration - doesn't work as well:

createSelectorCreator( defaultMemoize, <T extends FeatureState>(prevFeatureState: T, nextFeatureState: T) => // TS error: 'T' is not assignable to 'FeatureState' gatingFlagNames.every((f) => prevFeatureState.get(f) === nextFeatureState.get(f)), )

It seems that Reselect typings for createSelectorCreator API aren't robust enough.

vojtechszocs · 2020-03-05T18:10:41Z

frontend/public/components/nav/section.tsx

-    isNavItem,
-    isPerspective,
-  )(
+  withExtensions<NavSectionExtensionProps>({


As discussed before, this type param shouldn't be necessary, but I'd do that as a follow-up improvement.

That said, the withExtensions HOC is currently used on a single place, hopefully that number stays low.

vojtechszocs · 2020-03-05T18:30:45Z

frontend/public/components/nav/nav-header.tsx

@@ -104,7 +104,7 @@ const NavHeader_: React.FC<NavHeaderProps & StateProps> = ({

 const mapStateToProps = (state: RootState): StateProps => ({
  activePerspective: getActivePerspective(state),
-  flags: stateToFlagsObject(state),
+  flags: state[featureReducerName].toObject(),


vojtechszocs · 2020-03-05T18:31:54Z

frontend/public/reducers/features.ts

+export const stateToFlagsObject = (state: FeatureState, desiredFlags: string[]): FlagsObject =>
+  desiredFlags.reduce((allFlags, f) => ({ ...allFlags, [f]: state.get(f) }), {} as FlagsObject);


We can do that, but we should also differentiate between "empty flags" (empty array) vs. "all flags" (falsy value) cases.

I'll make desiredFlags optional and fallback to null which would allow us to reuse it like before 😃

christianvogt · 2020-03-05T23:31:44Z

/lgtm

openshift-ci-robot · 2020-03-05T23:32:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, 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~~ [christianvogt,vojtechszocs]

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

openshift-bot · 2020-03-05T23:39:48Z

/retest

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

openshift-bot · 2020-03-06T01:24:00Z

/retest

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

openshift-bot · 2020-03-06T01:49:48Z

/retest

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

vojtechszocs · 2020-03-06T13:01:37Z

/retest

openshift-ci-robot · 2020-03-06T14:44:55Z

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

Test name	Commit	Details	Rerun command
ci/prow/verify	`4cf4c7c`	link	`/test verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

vojtechszocs · 2020-03-06T15:25:14Z

/retest

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 5, 2020

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/sdk Related to console-plugin-sdk labels Feb 5, 2020

openshift-ci-robot requested review from christianvogt and jcaianirh February 5, 2020 18:23

This was referenced Feb 6, 2020

Memoize data derived from useExtensions #4232

Merged

Migrate all dashboard extensions #4233

Merged

christianvogt reviewed Mar 5, 2020

View reviewed changes

vojtechszocs commented Mar 5, 2020

View reviewed changes

Improve Console extension React hook & HOC

5755800

vojtechszocs force-pushed the update-ext-hook-hoc branch from 4cf4c7c to 5755800 Compare March 5, 2020 21:09

openshift-ci-robot assigned christianvogt Mar 5, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2020

openshift-merge-robot merged commit 4c022bf into openshift:master Mar 6, 2020

spadgett added this to the v4.5 milestone Mar 9, 2020

vojtechszocs mentioned this pull request Mar 16, 2020

Create watch k8s hook #3443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Console extension React hook & HOC #4211

Improve Console extension React hook & HOC #4211

vojtechszocs commented Feb 5, 2020

vojtechszocs commented Feb 5, 2020

vojtechszocs commented Feb 6, 2020

christianvogt Mar 5, 2020

vojtechszocs Mar 5, 2020

christianvogt Mar 5, 2020

vojtechszocs Mar 5, 2020

christianvogt Mar 5, 2020

christianvogt Mar 5, 2020

vojtechszocs Mar 5, 2020

christianvogt Mar 5, 2020

vojtechszocs Mar 5, 2020

vojtechszocs Mar 5, 2020

vojtechszocs Mar 5, 2020

vojtechszocs Mar 5, 2020

christianvogt commented Mar 5, 2020

openshift-ci-robot commented Mar 5, 2020

openshift-bot commented Mar 5, 2020

openshift-bot commented Mar 6, 2020

openshift-bot commented Mar 6, 2020

vojtechszocs commented Mar 6, 2020

openshift-ci-robot commented Mar 6, 2020 •

edited

vojtechszocs commented Mar 6, 2020

		export const stateToFlagsObject = (state: FeatureState, desiredFlags: string[]): FlagsObject =>
		desiredFlags.reduce((allFlags, f) => ({ ...allFlags, [f]: state.get(f) }), {} as FlagsObject);

Improve Console extension React hook & HOC #4211

Improve Console extension React hook & HOC #4211

Conversation

vojtechszocs commented Feb 5, 2020

vojtechszocs commented Feb 5, 2020

vojtechszocs commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Mar 5, 2020

openshift-ci-robot commented Mar 5, 2020

openshift-bot commented Mar 5, 2020

openshift-bot commented Mar 6, 2020

openshift-bot commented Mar 6, 2020

vojtechszocs commented Mar 6, 2020

openshift-ci-robot commented Mar 6, 2020 • edited

vojtechszocs commented Mar 6, 2020

openshift-ci-robot commented Mar 6, 2020 •

edited