address pr comments

Author: rileyajones

tensorboard/webapp/app_routing/location.ts

@@ -48,9 +48,6 @@ const utils = {
   getHref() {
     return window.location.href;
   },
-  getSearch() {
-    return window.location.search;
-  },
 };
 
 @Injectable()
@@ -60,7 +57,7 @@ export class Location implements LocationInterface {
   }
 
   getSearch(): SerializableQueryParams {
-    const searchParams = new URLSearchParams(utils.getSearch());
+    const searchParams = new URLSearchParams(window.location.search);
     const serializableSearchParams: SerializableQueryParams = [];
 
     // URLSearchParams is a Iterable but TypeScript does not know about that.

tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts

@@ -30,7 +30,6 @@ export type BaseFeatureFlagType = boolean | number | string | null | undefined;
 export type FeatureFlagType = BaseFeatureFlagType | Array<BaseFeatureFlagType>;
 
 export type FeatureFlagMetadata<T> = {
-  displayName: string;
   defaultValue?: T;
   queryParamOverride?: string;
   parseValue: (str: string) => T;
@@ -52,71 +51,57 @@ export const FeatureFlagMetadataMap: {
   [FlagName in keyof FeatureFlags]: FeatureFlagMetadata<FeatureFlags[FlagName]>;
 } = {
   scalarsBatchSize: {
-    displayName: 'scalarsBatchSize',
     queryParamOverride: SCALARS_BATCH_SIZE_PARAM_KEY,
     parseValue: parseInt,
   },
   enabledColorGroup: {
-    displayName: 'enabledColorGroup',
     queryParamOverride: ENABLE_COLOR_GROUP_QUERY_PARAM_KEY,
     parseValue: parseBoolean,
   },
   enabledColorGroupByRegex: {
-    displayName: 'enabledColorGroupByRegex',
     queryParamOverride: ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY,
     parseValue: parseBoolean,
   },
   enabledExperimentalPlugins: {
-    displayName: 'enabledExperimentalPlugins',
     queryParamOverride: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY,
     parseValue: (str: string) => [str],
     isArray: true,
   },
   enabledLinkedTime: {
-    displayName: 'enabledLinkedTime',
     queryParamOverride: ENABLE_LINKED_TIME_PARAM_KEY,
     parseValue: parseBoolean,
   },
   enabledCardWidthSetting: {
-    displayName: 'enabledCardWidthSetting',
     queryParamOverride: ENABLE_CARD_WIDTH_SETTING_PARAM_KEY,
     parseValue: parseBoolean,
   },
   enabledScalarDataTable: {
-    displayName: 'enabledScalarDataTable',
     queryParamOverride: ENABLE_DATA_TABLE_PARAM_KEY,
     parseValue: parseBoolean,
   },
   forceSvg: {
-    displayName: 'forceSvg',
     queryParamOverride: FORCE_SVG_RENDERER,
     parseValue: parseBoolean,
   },
   enableDarkModeOverride: {
-    displayName: 'enableDarkModeOverride',
     parseValue: parseBooleanOrNull,
   },
   defaultEnableDarkMode: {
-    displayName: 'defaultEnableDarkMode',
     queryParamOverride: ENABLE_DARK_MODE_QUERY_PARAM_KEY,
     parseValue: parseBoolean,
   },
   isAutoDarkModeAllowed: {
-    displayName: 'isAutoDarkModeAllowed',
     parseValue: parseBoolean,
   },
   inColab: {
-    displayName: 'inColab',
     defaultValue: false,
     queryParamOverride: 'tensorboardColab',
     parseValue: parseBoolean,
   },
   metricsImageSupportEnabled: {
-    displayName: 'metricsImageSupportEnabled',
     parseValue: parseBoolean,
   },
   enableTimeSeriesPromotion: {
-    displayName: 'enableTimeSeriesPromotion',
     parseValue: parseBoolean,
   },
 };

tensorboard/webapp/routes/dashboard_deeplink_provider.ts

@@ -24,10 +24,7 @@ import {
   FeatureFlagMetadataMap,
   FeatureFlagType,
 } from '../feature_flag/store/feature_flag_metadata';
-import {
-  getEnabledExperimentalPlugins,
-  getOverriddenFeatureFlags,
-} from '../feature_flag/store/feature_flag_selectors';
+import {getOverriddenFeatureFlags} from '../feature_flag/store/feature_flag_selectors';
 import {
   isPluginType,
   isSampledPlugin,
@@ -101,15 +98,6 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
           return [{key: TAG_FILTER_KEY, value: filterText}];
         })
       ),
-      store.select(getEnabledExperimentalPlugins).pipe(
-        map((enabledExperimentalPlugins) => {
-          return enabledExperimentalPlugins.map((pluginName) => ({
-            key: FeatureFlagMetadataMap.enabledExperimentalPlugins
-              .queryParamOverride!,
-            value: pluginName,
-          }));
-        })
-      ),
       store.select(getOverriddenFeatureFlags).pipe(
         map((featureFlags) => {
           return featureFlagsToSerializableQueryParams(

tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts

@@ -347,13 +347,15 @@ describe('core deeplink provider', () => {
     });
   });
 
+  /**
+   * These tests are intended to verify that feature flags are correctly serialized using
+   * featureFlagsToSerializableQueryParams
+   */
   describe('feature flag', () => {
     it('serializes enabled experimental plugins', () => {
-      store.overrideSelector(selectors.getEnabledExperimentalPlugins, [
-        'foo',
-        'bar',
-        'baz',
-      ]);
+      store.overrideSelector(selectors.getOverriddenFeatureFlags, {
+        enabledExperimentalPlugins: ['foo', 'bar', 'baz'],
+      });
       store.refreshState();
 
       expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
@@ -378,9 +380,9 @@ describe('core deeplink provider', () => {
       });
       store.refreshState();
 
-      expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
-        []
-      );
+      expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
+        {key: 'enableColorGroup', value: 'false'},
+      ]);
 
       store.overrideSelector(selectors.getOverriddenFeatureFlags, {});
       store.refreshState();
@@ -405,9 +407,9 @@ describe('core deeplink provider', () => {
       });
       store.refreshState();
 
-      expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
-        []
-      );
+      expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
+        {key: 'enableColorGroupByRegex', value: 'false'},
+      ]);
 
       store.overrideSelector(selectors.getOverriddenFeatureFlags, {});
       store.refreshState();

tensorboard/webapp/routes/feature_flag_serializer.ts

@@ -1,3 +1,17 @@
+/* Copyright 2022 The TensorFlow Authors. All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+==============================================================================*/
 import {SerializableQueryParams} from '../app_routing/types';
 import {FeatureFlagMetadata} from '../feature_flag/store/feature_flag_metadata';
 import {FeatureFlags} from '../feature_flag/types';
@@ -11,7 +25,7 @@ export function featureFlagsToSerializableQueryParams<T>(
       const key =
         featureFlagMetadataMap[featureFlag as keyof FeatureFlags]
           ?.queryParamOverride;
-      if (!key || !featureValue) {
+      if (!key || featureValue === undefined) {
         return [];
       }
       /**

tensorboard/webapp/routes/feature_flag_serializer_test.ts

@@ -1,3 +1,17 @@
+/* Copyright 2022 The TensorFlow Authors. All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+==============================================================================*/
 import {featureFlagsToSerializableQueryParams} from './feature_flag_serializer';
 
 describe('feature flag serializer', () => {
@@ -18,6 +32,7 @@ describe('feature flag serializer', () => {
         },
       };
     });
+
     it('should return empty list when no flags are overridden', () => {
       const serializableQueryParams = featureFlagsToSerializableQueryParams(
         {},
@@ -44,6 +59,23 @@ describe('feature flag serializer', () => {
       ]);
     });
 
+    it('should serialize feature flags with falsy values', () => {
+      const serializableQueryParams = featureFlagsToSerializableQueryParams(
+        {featureB: false, featureA: ''} as any,
+        featureFlagsMetadata
+      );
+      expect(serializableQueryParams).toEqual([
+        {
+          key: 'feature_b',
+          value: 'false',
+        },
+        {
+          key: 'feature_a',
+          value: '',
+        },
+      ]);
+    });
+
     it('should return multiple entries for features with array values', () => {
       const serializableQueryParams = featureFlagsToSerializableQueryParams(
         {featureA: 'a', featureB: ['foo', 'bar']} as any,