Feature: Add NgRx Actions, Reducers, Selectors for the Feature Flag Modal #5868

rileyajones · 2022-08-11T20:56:20Z

Motivation for features / changes

We want to have a modal to control the state of feature flags and persist their state to localStorage.
Creating such a modal is a large change and so, I'm breaking its creation into two PRs, this one and #5800.

This PR is just intended to prepare the NgRx and data source to be used the ui in #5800.

Technical description of changes

There are existing actions to handle getting and setting feature flag overrides, but none to remove an override. To resolve this, I'm adding two actions, one to reset a set number of overrides, and another to reset all overrides.

I'm also adding a selector to get the default state of all the feature flags. This should allow the modal to work in other flavors of TensorBoard which have different sets of feature flags without worrying about their specifics.

Screenshots of UI changes
No ui changes yet

Here's an example of what it may look like

JamesHollyer

I still need to review the tests. Here is what I have so far.

JamesHollyer · 2022-08-11T21:43:48Z

tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts

  }>()
 );
+
+export const resetFeatureFlagOverrides = createAction(


I have been told the name of the action should state what action has been taken as opposed to the effect that will take place. This is a subtle difference. Maybe change this to FlagSetToDefault or something along those lines.

What do you think of featureFlagOverridesReset?

tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts

tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts

bmd3k

Looks pretty good. I feel like there is something slightly off with how you've modeled your Actions (or I misunderstand the UI). And there are some missing tests.

bmd3k · 2022-08-15T19:16:22Z

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts

+    describe('resetAllPersistedFeatureFlags', () => {
+      it('removes entry from localStorage', () => {
+        const mockStorage: Record<string, string> = {};
+        spyOn(localStorage, 'getItem').and.callFake((key: string) => {


If James also prefers this test pattern (essentially using a fake instead of a mock) then it could be worth writing a cleanup PR to rewrite the existing tests to use this pattern, too.

I would prefer to not have a bunch of test infrastructure which simulates local storage and just test that the expected calls are made.

I felt like my version was a bit more thorough, but I've gone ahead and refactored it to make it consistent with the existing tests.

FWIW I preferred the use of fakes but +1 for consistency.

tensorboard/webapp/feature_flag/views/types.ts

tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts

bmd3k · 2022-08-15T19:28:01Z

tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts

  }>()
 );
+
+export const featureFlagOverridesReset = createAction(


I'm a little suspicious about featureFlagOverrideChanged and featureFlagOverridesReset living side x side. It either points to something wrong with your UI or something wrong with how you're modeling actions.

I am guessing that:

"featureFlagOverrideChanged" is fired by clicking on some sort of "Save" button at the bottom of the dialog.

"featureFlagsOverridesAllSetToDefault" is fired by clickign on some sort of "Reset All" button at the bottom of the dialog.

Is that right? If so, where does "featureFlagOverridesReset" fit in? Is there a third "Reset Selected" button or similar? That doesn't seem quite right.

featureFlagOverrideChanged is used when the user sets a flag to override as true or as false. However, the Partial is not able to indicate that a flag is set to default. So featureFlagOverridesReset handles the setting to default.

I was under the impression we would save these individually as the user changes the setting(ie there is no "save" button). However, if we do have a save button then featureFlagOverrideChanged(which should be named featureFlagOverrideSaved in this case) should be all that is necessary and the flags that are missing from the Partial object can all be set to default.

I think I prefer the option where we call these actions on individual changes and we do not require a save button.

For a bit of background I wanted to make featureFlagOverrideChanged have a single key and value pair but I could not get the type to work the way I wanted. We decided to use Partial as a compromise.

Thanks for the explanation, that makes sense. Yes agreed that it would be fine/preferable to omit the "Save" button.

I assumed there is a "Save" button because these actions specify lists of flags. Should they instead specify a single flag?

props<{ flag: keyof FeatureFlags }>()

I was trying to keep my new actions consistent with the existing ones. I believe the logic for using Partial<FeatureFlags> originally was to persist the key and value of the flag which lead to otherwise complicated typing.

Would it be possible, then, to also change the existing action? You could consider doing it in a followup PR if it seems out of scope.

This will work, keeps the typing sane, simplifies your effects and reducer code, and better models the user behavior:

export const featureFlagOverrideChanged = createAction( '[FEATURE FLAG] Store the feature flags in persistent localStorage', props<{ flag: keyof FeatureFlags; value: FeatureFlagType; }>() ); export const featureFlagOverridesReset = createAction( '[FEATURE FLAG] Resetting feature flag overrides', props<{ flag: keyof FeatureFlags; }>() );

(and, note, if you do change the props of featureFlagOverridesReset then it should be renamed to featureFlagOverrideReset (ie get rid of the plural from Overrides).

I am ok with this change if you feel strongly about it. We had discussed this as an option. The problem is that there is no way to guarantee the FeatureFlagType(which is basically a big OR statement with all the possible types) matches the type associated with the given key. It was all a lot cleaner and more strongly typed when we just used the Partial. I see the confusion this causes but, I still have a slight preference to the way it is now.

Yes, FeatureFlagType is unfortunately necessary to workaround some (relatively minor) limitations in the TypeScript type system.

I think in this case we should prefer the more-accurate data model over more-accurate type checks.

tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts

tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts

tensorboard/webapp/feature_flag/store/feature_flag_reducers.ts

JamesHollyer

LGTM

tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts

bmd3k · 2022-08-16T00:52:15Z

tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts

  }>()
 );
+
+export const featureFlagOverridesReset = createAction(


Would it be possible, then, to also change the existing action? You could consider doing it in a followup PR if it seems out of scope.

This will work, keeps the typing sane, simplifies your effects and reducer code, and better models the user behavior:

export const featureFlagOverrideChanged = createAction( '[FEATURE FLAG] Store the feature flags in persistent localStorage', props<{ flag: keyof FeatureFlags; value: FeatureFlagType; }>() ); export const featureFlagOverridesReset = createAction( '[FEATURE FLAG] Resetting feature flag overrides', props<{ flag: keyof FeatureFlags; }>() );

(and, note, if you do change the props of featureFlagOverridesReset then it should be renamed to featureFlagOverrideReset (ie get rid of the plural from Overrides).

tensorboard/webapp/feature_flag/store/feature_flag_reducers_test.ts

bmd3k · 2022-08-16T11:22:31Z

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts

+    describe('resetAllPersistedFeatureFlags', () => {
+      it('removes entry from localStorage', () => {
+        const mockStorage: Record<string, string> = {};
+        spyOn(localStorage, 'getItem').and.callFake((key: string) => {


FWIW I preferred the use of fakes but +1 for consistency.

…odal (tensorflow#5868) * add ngrx infrastructure for the feature-flags-modal

rileyajones requested a review from JamesHollyer August 11, 2022 20:56

rileyajones marked this pull request as ready for review August 11, 2022 20:56

rileyajones force-pushed the feature-flag-modal-infra branch from dfcce72 to ab64fb8 Compare August 11, 2022 20:58

rileyajones mentioned this pull request Aug 11, 2022

Show flags page as modal when showFlags query parameter is set #5800

Merged

rileyajones force-pushed the feature-flag-modal-infra branch 2 times, most recently from b821342 to 4153c5d Compare August 11, 2022 21:32

JamesHollyer reviewed Aug 11, 2022

View reviewed changes

rileyajones force-pushed the feature-flag-modal-infra branch from 7d5df11 to 31853ed Compare August 15, 2022 17:19

rileyajones requested review from JamesHollyer and bmd3k August 15, 2022 17:20

bmd3k reviewed Aug 15, 2022

View reviewed changes

rileyajones requested a review from bmd3k August 15, 2022 22:46

JamesHollyer approved these changes Aug 15, 2022

View reviewed changes

tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts Outdated Show resolved Hide resolved

bmd3k approved these changes Aug 16, 2022

View reviewed changes

rileyajones added 6 commits August 16, 2022 13:38

add ngrx infrastructure for the feature-flags-modal

01e6abc

rename actions

189bc63

pr comments

27ed84d

refactor test

fa4ff5c

remove unuseful tests

73c2911

rename action

9f7fecc

rileyajones force-pushed the feature-flag-modal-infra branch from d7d0edc to 9f7fecc Compare August 16, 2022 20:38

remove reference to feature flag removed in 5872

cfd3bd6

rileyajones merged commit 97aa10a into tensorflow:master Aug 17, 2022

yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023

Feature: Add NgRx Actions, Reducers, Selectors for the Feature Flag M…

e09ff39

…odal (tensorflow#5868) * add ngrx infrastructure for the feature-flags-modal

dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023

Feature: Add NgRx Actions, Reducers, Selectors for the Feature Flag M…

4b30e5c

…odal (tensorflow#5868) * add ngrx infrastructure for the feature-flags-modal

Feature: Add NgRx Actions, Reducers, Selectors for the Feature Flag Modal #5868

Feature: Add NgRx Actions, Reducers, Selectors for the Feature Flag Modal #5868

Uh oh!

Conversation

rileyajones commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for features / changes

Technical description of changes

Uh oh!

JamesHollyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bmd3k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesHollyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rileyajones commented Aug 11, 2022 •

edited

Loading