Skip to content

Commit 51b3a01

Browse files
authored
ng: make sure we do not push 'null' onto hash (tensorflow#3494)
In the Angular version of TensorBoard, we keep the activePlugin state in the Ngrx Store. In it, when activePlugin was never set, we use `null` to denote the emptiness. However, this null state was incompatible with how hash_storage works. tf_storage coerces the input, which wrongly coerced the null state (Angular build tools did not catch the wrong typing) in the activePlugin to "null" and set the hash as "#null". This change fixes the issue by converting `null` to '' which tf_storage understands to be the empty state.
1 parent 1ad7b3d commit 51b3a01

File tree

2 files changed

+32
-4
lines changed

2 files changed

+32
-4
lines changed

tensorboard/webapp/core/views/hash_storage_component.ts

+8-3
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ export class HashStorageComponent implements OnInit, OnChanges, OnDestroy {
4141
constructor(private readonly deepLinker: HashDeepLinker) {}
4242

4343
@Input()
44-
activePluginId!: string;
44+
activePluginId!: string | null;
4545

4646
@Output()
4747
onValueChange = new EventEmitter<{prop: ChangedProp; value: string}>();
@@ -74,11 +74,16 @@ export class HashStorageComponent implements OnInit, OnChanges, OnDestroy {
7474
ngOnChanges(changes: SimpleChanges) {
7575
if (changes['activePluginId']) {
7676
const activePluginIdChange = changes['activePluginId'];
77-
const option: SetStringOption = {};
77+
const option: SetStringOption = {defaultValue: ''};
7878
if (activePluginIdChange.firstChange) {
7979
option.useLocationReplace = true;
8080
}
81-
this.deepLinker.setPluginId(activePluginIdChange.currentValue, option);
81+
82+
const value =
83+
activePluginIdChange.currentValue === null
84+
? ''
85+
: activePluginIdChange.currentValue;
86+
this.deepLinker.setPluginId(value, option);
8287
}
8388
}
8489
}

tensorboard/webapp/core/views/hash_storage_test.ts

+24-1
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,29 @@ describe('hash storage test', () => {
7070
fixture.detectChanges();
7171
expect(setPluginIdSpy).toHaveBeenCalledWith('foo', {
7272
useLocationReplace: true,
73+
defaultValue: '',
74+
});
75+
});
76+
77+
it('sets the hash to empty string when activePlugin is not set', () => {
78+
store.overrideSelector(getActivePlugin, null);
79+
const fixture = TestBed.createComponent(HashStorageContainer);
80+
fixture.detectChanges();
81+
82+
expect(setPluginIdSpy).toHaveBeenCalledWith('', {
83+
useLocationReplace: true,
84+
defaultValue: '',
85+
});
86+
});
87+
88+
it('sets the hash to empty string when activePlugin is empty string', () => {
89+
store.overrideSelector(getActivePlugin, '');
90+
const fixture = TestBed.createComponent(HashStorageContainer);
91+
fixture.detectChanges();
92+
93+
expect(setPluginIdSpy).toHaveBeenCalledWith('', {
94+
useLocationReplace: true,
95+
defaultValue: '',
7396
});
7497
});
7598

@@ -84,7 +107,7 @@ describe('hash storage test', () => {
84107
fixture.detectChanges();
85108

86109
expect(setPluginIdSpy).toHaveBeenCalledTimes(2);
87-
expect(setPluginIdSpy).toHaveBeenCalledWith('bar', {});
110+
expect(setPluginIdSpy).toHaveBeenCalledWith('bar', jasmine.any(Object));
88111
});
89112

90113
it('dispatches plugin changed event when hash changes', () => {

0 commit comments

Comments
 (0)