Skip to content

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

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.

Sign up for GitHub

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

Merged
rileyajones merged 7 commits into from
Aug 17, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
01e6abc
add ngrx infrastructure for the feature-flags-modal
rileyajones Aug 11, 2022
189bc63
rename actions
rileyajones Aug 15, 2022
27ed84d
pr comments
rileyajones Aug 15, 2022
fa4ff5c
refactor test
rileyajones Aug 15, 2022
73c2911
remove unuseful tests
rileyajones Aug 16, 2022
9f7fecc
rename action
rileyajones Aug 16, 2022
cfd3bd6
remove reference to feature flag removed in 5872
rileyajones Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions tensorboard/webapp/feature_flag/actions/feature_flag_actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,14 @@ export const featureFlagOverrideChanged = createAction(
flags: Partial<FeatureFlags>;
}>()
);

export const featureFlagOverridesReset = createAction(
Copy link
Contributor

@bmd3k bmd3k Aug 15, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JamesHollyer JamesHollyer Aug 15, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JamesHollyer JamesHollyer Aug 15, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@bmd3k bmd3k Aug 15, 2022

Choose a reason for hiding this comment

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

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
}>()

Copy link
Contributor Author

@rileyajones rileyajones Aug 15, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@bmd3k bmd3k Aug 16, 2022

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@JamesHollyer JamesHollyer Aug 16, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@bmd3k bmd3k Aug 17, 2022

Choose a reason for hiding this comment

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

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.

'[FEATURE FLAG] Reset feature flag overrides',
props<{
flags: Array<keyof FeatureFlags>;
}>()
);

export const allFeatureFlagOverridesReset = createAction(
'[FEATURE FLAG] Reset all feature flag overrides'
);
1 change: 1 addition & 0 deletions tensorboard/webapp/feature_flag/effects/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ tf_ng_module(
deps = [
"//tensorboard/webapp:tb_polymer_interop_types",
"//tensorboard/webapp/feature_flag:force_svg_data_source",
"//tensorboard/webapp/feature_flag:types",
"//tensorboard/webapp/feature_flag/actions",
"//tensorboard/webapp/feature_flag/store",
"//tensorboard/webapp/feature_flag/store:types",
Expand Down
26 changes: 26 additions & 0 deletions tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ import {combineLatestWith, map, tap, withLatestFrom} from 'rxjs/operators';
import '../../tb_polymer_interop_types';
import {TBFeatureFlagDataSource} from '../../webapp_data_source/tb_feature_flag_data_source_types';
import {
allFeatureFlagOverridesReset,
featureFlagOverrideChanged,
featureFlagOverridesReset,
partialFeatureFlagsLoaded,
} from '../actions/feature_flag_actions';
import {ForceSvgDataSource} from '../force_svg_data_source';
Expand Down Expand Up @@ -95,6 +97,30 @@ export class FeatureFlagEffects {
{dispatch: false}
);

readonly resetFeatureFlagOverrides$ = createEffect(
() =>
this.actions$.pipe(
ofType(featureFlagOverridesReset),
tap(({flags}) => {
flags.forEach((flag) => {
this.dataSource.resetPersistedFeatureFlag(flag);
});
})
),
{dispatch: false}
);

readonly resetAllFeatureFlagOverrides$ = createEffect(
() =>
this.actions$.pipe(
ofType(allFeatureFlagOverridesReset),
tap(() => {
this.dataSource.resetAllPersistedFeatureFlags();
})
),
{dispatch: false}
);

constructor(
private readonly actions$: Actions,
private readonly store: Store<State>,
Expand Down
26 changes: 26 additions & 0 deletions tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ import {
TestingTBFeatureFlagDataSource,
} from '../../webapp_data_source/tb_feature_flag_testing';
import {
allFeatureFlagOverridesReset,
featureFlagOverrideChanged,
featureFlagOverridesReset,
partialFeatureFlagsLoaded,
} from '../actions/feature_flag_actions';
import {ForceSvgDataSource} from '../force_svg_data_source';
Expand Down Expand Up @@ -200,4 +202,28 @@ describe('feature_flag_effects', () => {
});
});
});

describe('resetFeatureFlagOverrides', () => {
it('calls resetPersistedFeatureFlag', () => {
const resetFlagSpy = spyOn(
dataSource,
'resetPersistedFeatureFlag'
).and.stub();
effects.resetFeatureFlagOverrides$.subscribe();
actions.next(featureFlagOverridesReset({flags: ['inColab']}));
expect(resetFlagSpy).toHaveBeenCalledOnceWith('inColab');
});
});

describe('resetAllFeatureFlagOverrides', () => {
it('calls resetAllPersistedFeatureFlags', () => {
const resetAllFlagsSpy = spyOn(
dataSource,
'resetAllPersistedFeatureFlags'
).and.stub();
effects.resetAllFeatureFlagOverrides$.subscribe();
actions.next(allFeatureFlagOverridesReset());
expect(resetAllFlagsSpy).toHaveBeenCalledOnceWith();
});
});
});
29 changes: 29 additions & 0 deletions tensorboard/webapp/feature_flag/store/feature_flag_reducers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ limitations under the License.
import {Action, createReducer, on} from '@ngrx/store';
import {globalSettingsLoaded, ThemeValue} from '../../persistent_settings';
import * as actions from '../actions/feature_flag_actions';
import {FeatureFlags} from '../types';
import {initialState} from './feature_flag_store_config_provider';
import {FeatureFlagState} from './feature_flag_types';

Expand Down Expand Up @@ -43,6 +44,34 @@ const reducer = createReducer<FeatureFlagState>(
},
};
}),
on(actions.featureFlagOverrideChanged, (state, newOverrides) => {
return {
...state,
flagOverrides: {
...state.flagOverrides,
...newOverrides.flags,
},
};
}),
on(actions.featureFlagOverridesReset, (state, overrides) => {
if (!overrides || !overrides.flags || !overrides.flags.length) {
return state;
}
const flagOverrides = {...state.flagOverrides};
overrides.flags.forEach((key) => {
delete flagOverrides[key as keyof FeatureFlags];
});
return {
...state,
flagOverrides,
};
}),
on(actions.allFeatureFlagOverridesReset, (state) => {
return {
...state,
flagOverrides: {},
};
}),
on(globalSettingsLoaded, (state, {partialSettings}) => {
if (!partialSettings.themeOverride) {
return state;
Expand Down
113 changes: 113 additions & 0 deletions tensorboard/webapp/feature_flag/store/feature_flag_reducers_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,119 @@ describe('feature_flag_reducers', () => {
});
});

describe('#featureFlagOverrideChanged', () => {
it('does not remove existing overrides', () => {
const prevState = buildFeatureFlagState({
flagOverrides: {
inColab: true,
defaultEnableDarkMode: true,
},
});
const nextState = reducers(
prevState,
actions.featureFlagOverrideChanged({
flags: {enabledLinkedTime: true},
})
);
expect(nextState.flagOverrides).toEqual({
inColab: true,
defaultEnableDarkMode: true,
enabledLinkedTime: true,
});
});

it('changes values of existing overrides', () => {
const prevState = buildFeatureFlagState({
flagOverrides: {
inColab: true,
defaultEnableDarkMode: true,
},
});
const nextState = reducers(
prevState,
actions.featureFlagOverrideChanged({
flags: {inColab: false},
})
);
expect(nextState.flagOverrides).toEqual({
inColab: false,
defaultEnableDarkMode: true,
});
});
});

describe('#featureFlagOverridesReset', () => {
it('does nothing when no overrides are provided', () => {
const prevState = buildFeatureFlagState();
const nextState = reducers(
prevState,
actions.featureFlagOverridesReset({flags: []})
);
// Intentionally using toBe rather than toEqual to ensure the object is the SAME object (not a copy).
expect(nextState).toBe(prevState);
});

it('removes all provided overrides', () => {
const prevState = buildFeatureFlagState({
flagOverrides: {
inColab: true,
forceSvg: true,
scalarsBatchSize: 5,
},
});
const nextState = reducers(
prevState,
actions.featureFlagOverridesReset({flags: ['forceSvg', 'inColab']})
);
expect(nextState.flagOverrides).toEqual({
scalarsBatchSize: 5,
});
});

it('does not effect default flags', () => {
const prevState = buildFeatureFlagState({
flagOverrides: {
inColab: true,
},
});

prevState.defaultFlags.inColab = true;
const nextState = reducers(
prevState,
actions.featureFlagOverridesReset({flags: ['inColab']})
);
expect(nextState.flagOverrides).toEqual({});
expect(nextState.defaultFlags).toBe(prevState.defaultFlags);
});
});

describe('#allFeatureFlagOverridesReset', () => {
it('always generates a new state', () => {
const prevState = buildFeatureFlagState({flagOverrides: {}});
const nextState = reducers(
prevState,
actions.allFeatureFlagOverridesReset()
);
expect(nextState.flagOverrides).not.toBe(prevState.flagOverrides);
expect(nextState.flagOverrides).toEqual({});
});

it('removes all overridden feature flags', () => {
const prevState = buildFeatureFlagState({
flagOverrides: {
inColab: true,
forceSvg: true,
scalarsBatchSize: 5,
},
});
const nextState = reducers(
prevState,
actions.allFeatureFlagOverridesReset()
);
expect(nextState.flagOverrides).toEqual({});
});
});

describe('#globalSettingsLoaded', () => {
it('sets dark mode overrides when global settings include it', () => {
const prevState = buildFeatureFlagState({
Expand Down
7 changes: 7 additions & 0 deletions tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ export const getFeatureFlags = createSelector(
}
);

export const getDefaultFeatureFlags = createSelector(
selectFeatureFlagState,
(state: FeatureFlagState): FeatureFlags => {
return state.defaultFlags;
}
);

export const getOverriddenFeatureFlags = createSelector(
selectFeatureFlagState,
(state: FeatureFlagState): Partial<FeatureFlags> => {
Expand Down
23 changes: 23 additions & 0 deletions tensorboard/webapp/feature_flag/store/feature_flag_selectors_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,29 @@ describe('feature_flag_selectors', () => {
});
});

describe('#getDefaultFeatureFlags', () => {
it('returns exact copy of the default feature flags', () => {
const initialState = buildFeatureFlagState();
const state = buildState(initialState);
const actual = selectors.getDefaultFeatureFlags(state);

expect(actual).toEqual(initialState.defaultFlags);
});

it('is uneffected by feature flag overrides', () => {
const initialState = buildFeatureFlagState({
flagOverrides: {
forceSvg: true,
inColab: true,
},
});
const state = buildState(initialState);
const actual = selectors.getDefaultFeatureFlags(state);

expect(actual).toEqual(initialState.defaultFlags);
});
});

describe('#getOverriddenFeatureFlags', () => {
it('returns empty object if it is not overridden', () => {
const state = buildState(buildFeatureFlagState());
Expand Down
4 changes: 4 additions & 0 deletions tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ export class FeatureFlagOverrideDataSource implements TBFeatureFlagDataSource {
);
}

resetAllPersistedFeatureFlags() {
localStorage.removeItem(FEATURE_FLAG_STORAGE_KEY);
}

getPersistentFeatureFlags(): Partial<FeatureFlags> {
const currentState = localStorage.getItem(FEATURE_FLAG_STORAGE_KEY);
if (currentState == null) {
Expand Down
15 changes: 15 additions & 0 deletions tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,5 +349,20 @@ describe('tb_feature_flag_data_source', () => {
);
});
});

describe('resetAllPersistedFeatureFlags', () => {
it('removes entry from localStorage', () => {
spyOn(localStorage, 'getItem').and.returnValue(
'{"enabledScalarDataTable":true}'
);
const setItemSpy = spyOn(localStorage, 'setItem').and.stub();
const removeItemSpy = spyOn(localStorage, 'removeItem').and.stub();

dataSource.resetAllPersistedFeatureFlags();
expect(removeItemSpy).toHaveBeenCalledOnceWith(
'tb_feature_flag_storage_key'
);
});
});
});
});
5 changes: 5 additions & 0 deletions tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ export abstract class TBFeatureFlagDataSource {
featureFlag: K
): void;

/**
* Removes all feature flags overridden in localStorage.
*/
abstract resetAllPersistedFeatureFlags(): void;

/**
* Gets the serialized data stored in localStorage for the stored feature
* flags.
Expand Down
2 changes: 2 additions & 0 deletions tensorboard/webapp/webapp_data_source/tb_feature_flag_testing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export class TestingTBFeatureFlagDataSource extends TBFeatureFlagDataSource {

resetPersistedFeatureFlag<K extends keyof FeatureFlags>(featureFlag: K) {}

resetAllPersistedFeatureFlags(): void {}

getPersistentFeatureFlags(): Partial<FeatureFlags> {
return {};
}
Expand Down