diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts index 1ed0713a72..74978aa997 100644 --- a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts +++ b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts @@ -12,7 +12,7 @@ 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 {Inject, Injectable} from '@angular/core'; +import {Injectable} from '@angular/core'; import {Actions, createEffect, ofType} from '@ngrx/effects'; import {Action, createAction, Store} from '@ngrx/store'; import {merge, Observable, of} from 'rxjs'; @@ -34,13 +34,17 @@ import { stateRehydratedFromUrl, } from '../actions'; import {AppRootProvider} from '../app_root'; -import {areRoutesEqual, getRouteId} from '../internal_utils'; +import { + areRoutesEqual, + getRouteId, + serializeCompareExperimentParams, +} from '../internal_utils'; import {Location} from '../location'; import {ProgrammaticalNavigationModule} from '../programmatical_navigation_module'; import {RouteConfigs} from '../route_config'; import {RouteRegistryModule} from '../route_registry_module'; import {getActiveRoute} from '../store/app_routing_selectors'; -import {Navigation, Route} from '../types'; +import {Navigation, Route, RouteKind, RouteParams} from '../types'; /** @typehack */ import * as _typeHackNgrxEffects from '@ngrx/effects'; /** @typehack */ import * as _typeHackModels from '@ngrx/store/src/models'; @@ -144,7 +148,26 @@ export class AppRoutingEffects { return nav !== null; }), map((programmaticalNavigation) => { - const {routeKind, routeParams} = programmaticalNavigation!; + const nav = programmaticalNavigation!; + const routeKind = nav.routeKind; + + // TODO(stephanwlee): currently, the RouteParams is ill-typed and you can + // currently add any property without any type error. Better type it. + let routeParams: RouteParams; + switch (nav.routeKind) { + case RouteKind.COMPARE_EXPERIMENT: + routeParams = { + experimentIds: serializeCompareExperimentParams( + nav.routeParams.aliasAndExperimentIds + ), + }; + break; + default: + routeParams = nav.routeParams; + } + return {routeKind, routeParams}; + }), + map(({routeKind, routeParams}) => { const routeMatch = this.routeConfigs ? this.routeConfigs.matchByRouteKind(routeKind, routeParams) : null; diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts index 7a2941b49d..1bc92dff5a 100644 --- a/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts +++ b/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts @@ -16,7 +16,7 @@ limitations under the License. import {Component} from '@angular/core'; import {fakeAsync, flush, TestBed, tick} from '@angular/core/testing'; import {provideMockActions} from '@ngrx/effects/testing'; -import {Action, createAction, Store} from '@ngrx/store'; +import {Action, createAction, props, Store} from '@ngrx/store'; import {MockStore, provideMockStore} from '@ngrx/store/testing'; import {of, ReplaySubject} from 'rxjs'; @@ -26,6 +26,7 @@ import {navigationRequested} from '../actions'; import {AppRootProvider, TestableAppRootProvider} from '../app_root'; import {Location} from '../location'; import { + NavigateToCompare, NavigateToExperiments, ProgrammaticalNavigationModule, } from '../programmatical_navigation_module'; @@ -40,6 +41,10 @@ import {AppRoutingEffects, TEST_ONLY} from './app_routing_effects'; class TestableComponent {} const testAction = createAction('[TEST] test actions'); +const testNavToCompareAction = createAction( + '[TEST] test nav to compare', + props() +); describe('app_routing_effects', () => { let effects: AppRoutingEffects; @@ -87,10 +92,10 @@ describe('app_routing_effects', () => { ]; } - function programmaticalNavigationFactory() { + function programmaticalNavToListviewFactory() { return { actionCreator: testAction, - lambda: (action: typeof testAction) => { + lambda: (action: ReturnType) => { return { routeKind: RouteKind.EXPERIMENTS, routeParams: {}, @@ -99,11 +104,26 @@ describe('app_routing_effects', () => { }; } + function programmaticalCompareNavigationFactory() { + return { + actionCreator: testNavToCompareAction, + lambda: (action: ReturnType) => { + return { + routeKind: RouteKind.COMPARE_EXPERIMENT, + routeParams: action, + } as NavigateToCompare; + }, + }; + } + await TestBed.configureTestingModule({ imports: [ RouteRegistryModule.registerRoutes(routeFactory), ProgrammaticalNavigationModule.registerProgrammaticalNavigation( - programmaticalNavigationFactory + programmaticalNavToListviewFactory + ), + ProgrammaticalNavigationModule.registerProgrammaticalNavigation( + programmaticalCompareNavigationFactory ), ], providers: [ @@ -539,6 +559,33 @@ describe('app_routing_effects', () => { }), ]); }); + + it('translates programmatical compare nav correctly', () => { + store.overrideSelector(getActiveRoute, null); + store.refreshState(); + + action.next( + testNavToCompareAction({ + aliasAndExperimentIds: [ + {alias: 'bar', id: 'foo'}, + {alias: 'omega', id: 'alpha'}, + ], + }) + ); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.COMPARE_EXPERIMENT, + params: { + experimentIds: 'bar:foo,omega:alpha', + }, + pathname: '/compare', + queryParams: [], + }), + }), + ]); + }); }); }); diff --git a/tensorboard/webapp/app_routing/internal_utils.ts b/tensorboard/webapp/app_routing/internal_utils.ts index a44b498479..51af5d3aeb 100644 --- a/tensorboard/webapp/app_routing/internal_utils.ts +++ b/tensorboard/webapp/app_routing/internal_utils.ts @@ -54,6 +54,12 @@ export function parseCompareExperimentStr( }); } +export function serializeCompareExperimentParams( + params: Array<{alias: string; id: string}> +): string { + return params.map(({alias, id}) => `${alias}:${id}`).join(','); +} + /** * Returns experimentIds from route parameter. For a route that does not contain * any experiment ids, it returns null. diff --git a/tensorboard/webapp/app_routing/internal_utils_test.ts b/tensorboard/webapp/app_routing/internal_utils_test.ts index 06f3c64834..90d09d48e8 100644 --- a/tensorboard/webapp/app_routing/internal_utils_test.ts +++ b/tensorboard/webapp/app_routing/internal_utils_test.ts @@ -44,6 +44,35 @@ describe('app_routing/utils', () => { }); }); + describe('#serializeCompareExperimentParams', () => { + it('serializes to empty string with an empty input', () => { + expect(utils.serializeCompareExperimentParams([])).toBe(''); + }); + + it('serializes alias and displayName', () => { + const input = [ + {alias: 'foo', id: 'bar'}, + {alias: 'baz', id: 'baz'}, + ]; + expect(utils.serializeCompareExperimentParams(input)).toBe( + 'foo:bar,baz:baz' + ); + }); + + it('does not deduplicate or validate', () => { + const input = [ + {alias: 'foo', id: 'bar'}, + // does not deduplicate + {alias: 'tar', id: 'bar'}, + {alias: 'foo', id: 'bang'}, + {alias: '', id: ''}, + ]; + expect(utils.serializeCompareExperimentParams(input)).toBe( + 'foo:bar,tar:bar,foo:bang,:' + ); + }); + }); + describe('#getExperimentIdsFromRouteParams', () => { it('returns ids from compare route', () => { const actual = utils.getExperimentIdsFromRouteParams( diff --git a/tensorboard/webapp/app_routing/programmatical_navigation_module_test.ts b/tensorboard/webapp/app_routing/programmatical_navigation_module_test.ts index ed532cc6db..53399f622d 100644 --- a/tensorboard/webapp/app_routing/programmatical_navigation_module_test.ts +++ b/tensorboard/webapp/app_routing/programmatical_navigation_module_test.ts @@ -101,11 +101,18 @@ describe('programmatical navigation module test', () => { function provider2() { return { actionCreator: testAction, - lambda: (action: typeof testAction) => { + lambda: (action: typeof testAction): NavigateToCompare => { return { routeKind: RouteKind.COMPARE_EXPERIMENT, - routeParams: {experimentIds: 'foo'}, - } as NavigateToCompare; + routeParams: { + aliasAndExperimentIds: [ + { + alias: 'Foo', + id: 'foo', + }, + ], + }, + }; }, }; } diff --git a/tensorboard/webapp/app_routing/programmatical_navigation_types.ts b/tensorboard/webapp/app_routing/programmatical_navigation_types.ts index de1d788c71..9268f9dba4 100644 --- a/tensorboard/webapp/app_routing/programmatical_navigation_types.ts +++ b/tensorboard/webapp/app_routing/programmatical_navigation_types.ts @@ -15,7 +15,7 @@ limitations under the License. import {InjectionToken} from '@angular/core'; import {Action, ActionCreator, Creator} from '@ngrx/store'; -import {CompareRouteParams, ExperimentRouteParams, RouteKind} from './types'; +import {ExperimentRouteParams, RouteKind} from './types'; export const NAVIGATION_PROVIDER = new InjectionToken( '[App Routing] Programmatical Navigation Provider' @@ -28,7 +28,9 @@ export interface NavigateToExperiment { export interface NavigateToCompare { routeKind: RouteKind.COMPARE_EXPERIMENT; - routeParams: CompareRouteParams; + routeParams: { + aliasAndExperimentIds: Array<{alias: string; id: string}>; + }; } export interface NavigateToExperiments {