Skip to content

Commit 6f28538

Browse files
authored
Bug Fix: Remove all forceSvg specific logic (#6244)
## Motivation for features / changes `forceSvg` being stored to localStorage predates our feature allowing any feature flag to be stored to localStorage. Unfortunately these two features work quite poorly together. The `forceSvg` flag has turned into a kind of virus because it writes itself to localStorage when it appears in the url. This means that users who having it enabled sharing a link to another user will cause the setting to spread. Now that there is a new way of enabling this feature permanently for users, I am removing the older method of persisting the flag. ## Technical description of changes ## Screenshots of UI changes None ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Navigate to localhost:6006?showFlags 3) Ensure `forceSvg` is disabled 4) Navigate to localhost:6006?showFlags&forceSvg=true 5) Ensure `forceSvg` is enabled 6) Navigate to localhost:6006?showFlags 7) Ensure `forceSvg` is still disabled ## Alternate designs / implementations considered I could have just updated the `force_svg_data_source` to stop storing force_svg differently and potentially migrate users to store `forceSvg` using the same persistence as other feature flags. However, we do not believe there are a lot of users that actually need the feature and thus we would prefer to force the small number of users that need this feature to re-enable it while rescuing unintentionally effected users.
1 parent 7daa225 commit 6f28538

File tree

8 files changed

+2
-233
lines changed

8 files changed

+2
-233
lines changed

tensorboard/webapp/feature_flag/BUILD

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ tf_ng_module(
88
"feature_flag_module.ts",
99
],
1010
deps = [
11-
":force_svg_data_source",
1211
"//tensorboard/webapp/angular:expect_angular_common_http",
1312
"//tensorboard/webapp/feature_flag/effects",
1413
"//tensorboard/webapp/feature_flag/http",
@@ -22,18 +21,6 @@ tf_ng_module(
2221
],
2322
)
2423

25-
tf_ng_module(
26-
name = "force_svg_data_source",
27-
srcs = [
28-
"force_svg_data_source.ts",
29-
"force_svg_data_source_module.ts",
30-
],
31-
deps = [
32-
"//tensorboard/webapp/webapp_data_source:feature_flag_types",
33-
"@npm//@angular/core",
34-
],
35-
)
36-
3724
tf_ts_library(
3825
name = "types",
3926
srcs = [
@@ -53,19 +40,6 @@ tf_ts_library(
5340
],
5441
)
5542

56-
tf_ts_library(
57-
name = "test_lib",
58-
testonly = True,
59-
srcs = [
60-
"force_svg_data_source_test.ts",
61-
],
62-
deps = [
63-
":force_svg_data_source",
64-
"//tensorboard/webapp/angular:expect_angular_core_testing",
65-
"@npm//@types/jasmine",
66-
],
67-
)
68-
6943
tf_ng_web_test_suite(
7044
name = "karma_test",
7145
deps = [

tensorboard/webapp/feature_flag/effects/BUILD

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ tf_ng_module(
99
],
1010
deps = [
1111
"//tensorboard/webapp:tb_polymer_interop_types",
12-
"//tensorboard/webapp/feature_flag:force_svg_data_source",
1312
"//tensorboard/webapp/feature_flag:types",
1413
"//tensorboard/webapp/feature_flag/actions",
1514
"//tensorboard/webapp/feature_flag/store",
@@ -33,7 +32,6 @@ tf_ng_module(
3332
":effects",
3433
"//tensorboard/webapp/angular:expect_angular_core_testing",
3534
"//tensorboard/webapp/angular:expect_ngrx_store_testing",
36-
"//tensorboard/webapp/feature_flag:force_svg_data_source",
3735
"//tensorboard/webapp/feature_flag:testing",
3836
"//tensorboard/webapp/feature_flag:types",
3937
"//tensorboard/webapp/feature_flag/actions",

tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import {
2525
featureFlagOverridesReset,
2626
partialFeatureFlagsLoaded,
2727
} from '../actions/feature_flag_actions';
28-
import {ForceSvgDataSource} from '../force_svg_data_source';
2928
import {
3029
getFeatureFlags,
3130
getFeatureFlagsMetadata,
@@ -58,12 +57,6 @@ export class FeatureFlagEffects {
5857
featureFlagsMetadata
5958
);
6059

61-
if (features.forceSvg != null) {
62-
this.forceSvgDataSource.updateForceSvgFlag(features.forceSvg);
63-
} else if (this.forceSvgDataSource.getForceSvgFlag()) {
64-
features.forceSvg = true;
65-
}
66-
6760
return partialFeatureFlagsLoaded({features});
6861
})
6962
)
@@ -138,8 +131,7 @@ export class FeatureFlagEffects {
138131
constructor(
139132
private readonly actions$: Actions,
140133
private readonly store: Store<State>,
141-
private readonly dataSource: TBFeatureFlagDataSource,
142-
private readonly forceSvgDataSource: ForceSvgDataSource
134+
private readonly dataSource: TBFeatureFlagDataSource
143135
) {}
144136

145137
/** @export */

tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts

Lines changed: 1 addition & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@ import {
2929
featureFlagOverridesReset,
3030
partialFeatureFlagsLoaded,
3131
} from '../actions/feature_flag_actions';
32-
import {ForceSvgDataSource} from '../force_svg_data_source';
33-
import {ForceSvgDataSourceModule} from '../force_svg_data_source_module';
3432
import {FeatureFlagMetadataMap} from '../store/feature_flag_metadata';
3533
import {
3634
getFeatureFlags,
@@ -40,21 +38,19 @@ import {
4038
} from '../store/feature_flag_selectors';
4139
import {State} from '../store/feature_flag_types';
4240
import {buildFeatureFlag} from '../testing';
43-
import {FeatureFlags} from '../types';
4441
import {FeatureFlagEffects} from './feature_flag_effects';
4542

4643
describe('feature_flag_effects', () => {
4744
let actions: ReplaySubject<Action>;
4845
let store: MockStore<State>;
4946
let dataSource: TestingTBFeatureFlagDataSource;
50-
let forceSvgDataSource: ForceSvgDataSource;
5147
let effects: FeatureFlagEffects;
5248
let setPolymerFeatureFlagsSpy: jasmine.Spy;
5349

5450
beforeEach(async () => {
5551
actions = new ReplaySubject<Action>(1);
5652
await TestBed.configureTestingModule({
57-
imports: [TBFeatureFlagTestingModule, ForceSvgDataSourceModule],
53+
imports: [TBFeatureFlagTestingModule],
5854
providers: [
5955
provideMockActions(actions),
6056
FeatureFlagEffects,
@@ -74,7 +70,6 @@ describe('feature_flag_effects', () => {
7470
effects = TestBed.inject(FeatureFlagEffects);
7571
store = TestBed.inject<Store<State>>(Store) as MockStore<State>;
7672
dataSource = TestBed.inject(TestingTBFeatureFlagDataSource);
77-
forceSvgDataSource = TestBed.inject(ForceSvgDataSource);
7873
store.overrideSelector(getIsAutoDarkModeAllowed, false);
7974
store.overrideSelector(getFeatureFlagsMetadata, FeatureFlagMetadataMap);
8075
});
@@ -112,56 +107,6 @@ describe('feature_flag_effects', () => {
112107
}),
113108
]);
114109
});
115-
116-
it('calls updateForceSvgFlag when getFeatures returns a value for forceSvg', () => {
117-
spyOn(dataSource, 'getFeatures').and.returnValue(
118-
buildFeatureFlag({
119-
forceSvg: true,
120-
})
121-
);
122-
let updateSpy = spyOn(
123-
forceSvgDataSource,
124-
'updateForceSvgFlag'
125-
).and.stub();
126-
127-
actions.next(effects.ngrxOnInitEffects());
128-
129-
expect(recordedActions).toEqual([
130-
partialFeatureFlagsLoaded({
131-
features: buildFeatureFlag({
132-
forceSvg: true,
133-
}),
134-
}),
135-
]);
136-
137-
expect(updateSpy).toHaveBeenCalledOnceWith(true);
138-
});
139-
140-
it('gets forceSVG flag from ForceSvgDataSource when getFeatures returns no value for forceSvg', () => {
141-
let featureFlags: Partial<FeatureFlags> = buildFeatureFlag();
142-
delete featureFlags.forceSvg;
143-
spyOn(dataSource, 'getFeatures').and.returnValue(featureFlags);
144-
let updateSpy = spyOn(
145-
forceSvgDataSource,
146-
'updateForceSvgFlag'
147-
).and.stub();
148-
let getSpy = spyOn(forceSvgDataSource, 'getForceSvgFlag').and.returnValue(
149-
true
150-
);
151-
152-
actions.next(effects.ngrxOnInitEffects());
153-
154-
expect(recordedActions).toEqual([
155-
partialFeatureFlagsLoaded({
156-
features: buildFeatureFlag({
157-
forceSvg: true,
158-
}),
159-
}),
160-
]);
161-
162-
expect(getSpy).toHaveBeenCalledOnceWith();
163-
expect(updateSpy).toHaveBeenCalledTimes(0);
164-
});
165110
});
166111

167112
describe('updatePolymerFeatureFlags$', () => {

tensorboard/webapp/feature_flag/feature_flag_module.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import {
2424
} from '../persistent_settings';
2525
import {TBFeatureFlagModule} from '../webapp_data_source/tb_feature_flag_module';
2626
import {FeatureFlagEffects} from './effects/feature_flag_effects';
27-
import {ForceSvgDataSourceModule} from './force_svg_data_source_module';
2827
import {FeatureFlagHttpInterceptor} from './http/feature_flag_http_interceptor';
2928
import {reducers} from './store/feature_flag_reducers';
3029
import {getEnableDarkModeOverride} from './store/feature_flag_selectors';
@@ -47,7 +46,6 @@ export function getThemeSettingSelector() {
4746

4847
@NgModule({
4948
imports: [
50-
ForceSvgDataSourceModule,
5149
TBFeatureFlagModule,
5250
StoreModule.forFeature(
5351
FEATURE_FLAG_FEATURE_KEY,

tensorboard/webapp/feature_flag/force_svg_data_source.ts

Lines changed: 0 additions & 41 deletions
This file was deleted.

tensorboard/webapp/feature_flag/force_svg_data_source_module.ts

Lines changed: 0 additions & 22 deletions
This file was deleted.

tensorboard/webapp/feature_flag/force_svg_data_source_test.ts

Lines changed: 0 additions & 75 deletions
This file was deleted.

0 commit comments

Comments
 (0)