diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts index 74978aa997..bd93a4366b 100644 --- a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts +++ b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts @@ -197,8 +197,8 @@ export class AppRoutingEffects { /** * @export */ - fireNavigatedIfValidRoute$ = createEffect(() => { - return this.validatedRoute$.pipe( + navigate$ = createEffect(() => { + const dispatchNavigating$ = this.validatedRoute$.pipe( tap(({routeMatch, options}) => { if (options.browserInitiated && routeMatch.deepLinkProvider) { const rehydratingState = routeMatch.deepLinkProvider.deserializeQueryParams( @@ -258,75 +258,72 @@ export class AppRoutingEffects { // sequentially. this.store.dispatch(navigating({after: route})); }), - // Let the router-outlet flush the change in a microtask. - debounceTime(0), + // Inject some async-ness so: + // 1. the router-outlet flush the change in a microtask. + // 2. we do not have composite action (synchronous dispatchment of + // actions). + debounceTime(0) + ); + + const changeUrl$ = dispatchNavigating$.pipe( + withLatestFrom(this.store.select(getActiveRoute)), + map(([nextRoute, oldRoute]) => { + // The URL hash can be set via HashStorageComponent (which uses + // Polymer's tf-storage). DeepLinkProviders also modify the URL when + // a provider's serializeStateToQueryParams() emits. These result in + // the URL updated without the previous hash. HashStorageComponent + // makes no attempt to restore the hash, so it is dropped. + + // This results in bad behavior when refreshing (e.g. lost active + // plugin) and when changing dashboards (e.g. lost tagFilter). + + // TODO(b/169799696): either AppRouting should manage the URL entirely + // (including hash), or we make the app wait for AppRouting to + // initialize before setting the active plugin hash. + // See https://github.com/tensorflow/tensorboard/issues/4207. + const preserveHash = + oldRoute === null || + nextRoute === null || + getRouteId(oldRoute.routeKind, oldRoute.params) === + getRouteId(nextRoute.routeKind, nextRoute.params); + return { + preserveHash, + route: nextRoute, + }; + }), + tap(({preserveHash, route}) => { + const shouldUpdateHistory = !areRoutesEqual(route, { + pathname: this.appRootProvider.getAppRootlessPathname( + this.location.getPath() + ), + queryParams: this.location.getSearch(), + }); + if (!shouldUpdateHistory) return; + + if (route.navigationOptions.replaceState) { + this.location.replaceState( + this.appRootProvider.getAbsPathnameWithAppRoot( + this.location.getFullPathFromRouteOrNav(route, preserveHash) + ) + ); + } else { + this.location.pushState( + this.appRootProvider.getAbsPathnameWithAppRoot( + this.location.getFullPathFromRouteOrNav(route, preserveHash) + ) + ); + } + }) + ); + + return changeUrl$.pipe( withLatestFrom(this.store.select(getActiveRoute)), - map(([route, oldRoute]) => { + map(([{route}, oldRoute]) => { return navigated({before: oldRoute, after: route}); }) ); }); - // TODO(stephanwlee): move this to a "view". - /** @export */ - changeBrowserUrl$ = createEffect( - () => { - return this.actions$.pipe( - ofType(navigated), - withLatestFrom(this.store.select(getActiveRoute)), - filter(([, route]) => Boolean(route)), - map(([navigatedAction, route]) => { - // The URL hash can be set via HashStorageComponent (which uses - // Polymer's tf-storage). DeepLinkProviders also modify the URL when - // a provider's serializeStateToQueryParams() emits. These result in - // the URL updated without the previous hash. HashStorageComponent - // makes no attempt to restore the hash, so it is dropped. - - // This results in bad behavior when refreshing (e.g. lost active - // plugin) and when changing dashboards (e.g. lost tagFilter). - - // TODO(b/169799696): either AppRouting should manage the URL entirely - // (including hash), or we make the app wait for AppRouting to - // initialize before setting the active plugin hash. - // See https://github.com/tensorflow/tensorboard/issues/4207. - const oldRoute = navigatedAction.before; - const preserveHash = - oldRoute === null || - getRouteId(oldRoute.routeKind, oldRoute.params) === - getRouteId(route!.routeKind, route!.params); - return { - preserveHash, - route: route!, - }; - }), - filter(({route}) => { - return !areRoutesEqual(route, { - pathname: this.appRootProvider.getAppRootlessPathname( - this.location.getPath() - ), - queryParams: this.location.getSearch(), - }); - }), - tap(({preserveHash, route}) => { - if (route.navigationOptions.replaceState) { - this.location.replaceState( - this.appRootProvider.getAbsPathnameWithAppRoot( - this.location.getFullPathFromRouteOrNav(route, preserveHash) - ) - ); - } else { - this.location.pushState( - this.appRootProvider.getAbsPathnameWithAppRoot( - this.location.getFullPathFromRouteOrNav(route, preserveHash) - ) - ); - } - }) - ); - }, - {dispatch: false} - ); - /** @export */ ngrxOnInitEffects(): Action { return initAction(); 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 1bc92dff5a..40e253ff6d 100644 --- a/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts +++ b/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts @@ -153,7 +153,7 @@ describe('app_routing_effects', () => { store.overrideSelector(getActiveRoute, null); }); - describe('fireNavigatedIfValidRoute$', () => { + describe('navigate$', () => { let actualActions: Action[]; beforeEach(() => { @@ -163,7 +163,7 @@ describe('app_routing_effects', () => { spyOn(store, 'dispatch').and.callFake((action: Action) => { actualActions.push(action); }); - effects.fireNavigatedIfValidRoute$.subscribe((action) => { + effects.navigate$.subscribe((action) => { actualActions.push(action); }); }); @@ -233,6 +233,52 @@ describe('app_routing_effects', () => { ]); }); + describe('order of events', () => { + it( + 'dispatches navigating, waits (for UI to clear prev route page), ' + + 'changes url, then dispatches navigated', + fakeAsync(() => { + store.overrideSelector(getActiveRoute, null); + store.refreshState(); + + action.next( + actions.navigationRequested({ + pathname: '/experiments', + }) + ); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + params: {}, + pathname: '/experiments', + queryParams: [], + }), + }), + ]); + expect(pushStateSpy).not.toHaveBeenCalled(); + + tick(); + + expect(pushStateSpy).toHaveBeenCalledOnceWith('/experiments'); + + expect(actualActions).toEqual([ + jasmine.any(Object), + actions.navigated({ + before: null, + after: buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + params: {}, + pathname: '/experiments', + queryParams: [], + }), + }), + ]); + }) + ); + }); + describe('deeplink reads', () => { beforeEach(() => { store.overrideSelector(getActiveRoute, null); @@ -587,190 +633,183 @@ describe('app_routing_effects', () => { ]); }); }); - }); - - describe('changeBrowserUrl$', () => { - let replaceStateSpy: jasmine.Spy; - beforeEach(() => { - effects = TestBed.inject(AppRoutingEffects); - effects.changeBrowserUrl$.subscribe(() => {}); - replaceStateSpy = spyOn(location, 'replaceState'); - }); + describe('url changes', () => { + let replaceStateSpy: jasmine.Spy; - it('noops if the new route matches current URL', () => { - const activeRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENTS, - pathname: '/experiments', - queryParams: [], - navigationOptions: { - replaceState: false, - }, + beforeEach(() => { + replaceStateSpy = spyOn(location, 'replaceState'); }); - store.overrideSelector(getActiveRoute, activeRoute); - store.refreshState(); - getHashSpy.and.returnValue(''); - getPathSpy.and.returnValue('/experiments'); - getSearchSpy.and.returnValue([]); - action.next( - actions.navigated({ - before: null, - after: activeRoute, - }) - ); + function navigateAndExpect( + navigation: Navigation | Route, + expected: {pushStateUrl: null | string; replaceStateUrl: null | string} + ) { + fakeAsync(() => { + action.next(actions.navigationRequested(navigation)); - expect(pushStateSpy).not.toHaveBeenCalled(); - expect(replaceStateSpy).not.toHaveBeenCalled(); - }); + tick(); + if (expected.pushStateUrl === null) { + expect(pushStateSpy).not.toHaveBeenCalled(); + } else { + expect(pushStateSpy).toHaveBeenCalledWith(expected.pushStateUrl); + } + + if (expected.replaceStateUrl === null) { + expect(replaceStateSpy).not.toHaveBeenCalled(); + } else { + expect(replaceStateSpy).toHaveBeenCalledWith( + expected.replaceStateUrl + ); + } + })(); + } + + it('noops if the new route matches current URL', () => { + const activeRoute = buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + pathname: '/experiments', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }); + store.overrideSelector(getActiveRoute, activeRoute); + store.refreshState(); + getHashSpy.and.returnValue(''); + getPathSpy.and.returnValue('/experiments'); + getSearchSpy.and.returnValue([]); - it('pushes state if path and search do not match new route on navigated', () => { - const activeRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENTS, - pathname: '/experiments', - queryParams: [], - navigationOptions: { - replaceState: false, - }, + navigateAndExpect(activeRoute, { + pushStateUrl: null, + replaceStateUrl: null, + }); }); - store.overrideSelector(getActiveRoute, activeRoute); - store.refreshState(); - getHashSpy.and.returnValue(''); - getPathSpy.and.returnValue('meow'); - getSearchSpy.and.returnValue([]); - action.next( - actions.navigated({ - before: null, - after: activeRoute, - }) - ); - - expect(pushStateSpy).toHaveBeenCalledWith('/experiments'); - }); + it('pushes state if path and search do not match new route on navigated', () => { + const activeRoute = buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + pathname: '/experiments', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }); + store.overrideSelector(getActiveRoute, activeRoute); + store.refreshState(); + getHashSpy.and.returnValue(''); + getPathSpy.and.returnValue('meow'); + getSearchSpy.and.returnValue([]); - it('replaces state if route navigationOption says so', () => { - const activeRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENTS, - pathname: '/experiments', - queryParams: [], - navigationOptions: { - replaceState: true, - }, + navigateAndExpect( + {pathname: '/experiment/123'}, + { + pushStateUrl: '/experiment/123', + replaceStateUrl: null, + } + ); }); - store.overrideSelector(getActiveRoute, activeRoute); - store.refreshState(); - getHashSpy.and.returnValue(''); - getPathSpy.and.returnValue('meow'); - getSearchSpy.and.returnValue([]); - action.next( - actions.navigated({ - before: null, - after: activeRoute, - }) - ); - - expect(pushStateSpy).not.toHaveBeenCalled(); - expect(replaceStateSpy).toHaveBeenCalledWith('/experiments'); - }); + it('replaces state if route navigationOption says so', () => { + const activeRoute = buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + pathname: '/experiments', + queryParams: [], + navigationOptions: { + replaceState: true, + }, + }); + store.overrideSelector(getActiveRoute, activeRoute); + store.refreshState(); + getHashSpy.and.returnValue(''); + getPathSpy.and.returnValue('meow'); + getSearchSpy.and.returnValue([]); - it('preserves hash upon replace for initial navigation', () => { - const activeRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENTS, - pathname: '/experiments', - queryParams: [], - navigationOptions: { - replaceState: true, - }, + navigateAndExpect( + {pathname: '/experiments', replaceState: true}, + { + pushStateUrl: null, + replaceStateUrl: '/experiments', + } + ); }); - store.overrideSelector(getActiveRoute, activeRoute); - store.refreshState(); - getHashSpy.and.returnValue('#foo'); - getPathSpy.and.returnValue('meow'); - getSearchSpy.and.returnValue([]); - - action.next( - actions.navigated({ - before: null, - after: activeRoute, - }) - ); - expect(replaceStateSpy).toHaveBeenCalledWith('/experiments#foo'); - }); + it('preserves hash upon replace for initial navigation', () => { + store.overrideSelector(getActiveRoute, null); + store.refreshState(); + getHashSpy.and.returnValue('#foo'); + getPathSpy.and.returnValue('meow'); + getSearchSpy.and.returnValue([]); - // This hash preservation spec may become obsolete. If we enable app_routing - // to properly set the URL hash, and all TB embedders use app_routing, then - // this spec can be removed. - it('preserves hash upon navigations to the same route id', () => { - const activeRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENT, - pathname: '/experiment', - queryParams: [], - navigationOptions: { - replaceState: true, - }, - }); - const nextActiveRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENT, - pathname: '/experiment', - queryParams: [{key: 'q', value: 'new_value'}], - navigationOptions: { - replaceState: true, - }, + navigateAndExpect( + {pathname: '/experiments', replaceState: true}, + { + pushStateUrl: null, + replaceStateUrl: '/experiments#foo', + } + ); }); - store.overrideSelector(getActiveRoute, nextActiveRoute); - store.refreshState(); - getHashSpy.and.returnValue('#foo'); - getPathSpy.and.returnValue('meow'); - getSearchSpy.and.returnValue([]); - - action.next( - actions.navigated({ - before: activeRoute, - after: nextActiveRoute, - }) - ); - expect(replaceStateSpy).toHaveBeenCalledWith( - '/experiment?q=new_value#foo' - ); - }); + // This hash preservation spec may become obsolete. If we enable app_routing + // to properly set the URL hash, and all TB embedders use app_routing, then + // this spec can be removed. + it('preserves hash upon navigations to the same route id', () => { + const activeRoute = buildRoute({ + routeKind: RouteKind.EXPERIMENT, + pathname: '/experiment', + params: {experimentId: '123'}, + queryParams: [], + navigationOptions: { + replaceState: true, + }, + }); + const nextActiveRoute = buildRoute({ + routeKind: RouteKind.EXPERIMENT, + pathname: '/experiment', + queryParams: [{key: 'q', value: 'new_value'}], + navigationOptions: { + replaceState: true, + }, + }); + store.overrideSelector(getActiveRoute, activeRoute); + store.refreshState(); + getHashSpy.and.returnValue('#foo'); + getPathSpy.and.returnValue('meow'); + getSearchSpy.and.returnValue([]); - it('discards hash upon navigations to a new route id', () => { - const activeRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENTS, - pathname: '/experiments', - queryParams: [], - navigationOptions: { - replaceState: true, - }, - }); - const nextActiveRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENT, - pathname: '/experiment', - // Changing route params produces a new route id. - params: {experimentId: '123'}, - queryParams: [], - navigationOptions: { - replaceState: true, - }, + navigateAndExpect( + {pathname: '/experiment/123', replaceState: true}, + { + pushStateUrl: null, + replaceStateUrl: '/experiment/123#foo', + } + ); }); - store.overrideSelector(getActiveRoute, nextActiveRoute); - store.refreshState(); - getHashSpy.and.returnValue('#foo'); - getPathSpy.and.returnValue('meow'); - getSearchSpy.and.returnValue([]); - action.next( - actions.navigated({ - before: activeRoute, - after: nextActiveRoute, - }) - ); + it('discards hash upon navigations to a new route id', () => { + const activeRoute = buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + pathname: '/experiments', + queryParams: [], + navigationOptions: { + replaceState: true, + }, + }); - expect(replaceStateSpy).toHaveBeenCalledWith('/experiment'); + store.overrideSelector(getActiveRoute, activeRoute); + store.refreshState(); + getHashSpy.and.returnValue('#foo'); + getPathSpy.and.returnValue('meow'); + getSearchSpy.and.returnValue([]); + + navigateAndExpect( + {pathname: '/experiment/123', replaceState: true}, + { + pushStateUrl: null, + replaceStateUrl: '/experiment/123', + } + ); + }); }); }); @@ -783,7 +822,7 @@ describe('app_routing_effects', () => { effects = TestBed.inject(AppRoutingEffects); const dispatchSpy = spyOn(store, 'dispatch'); - effects.fireNavigatedIfValidRoute$.subscribe((action) => { + effects.navigate$.subscribe((action) => { actualActions.push(action); }); @@ -791,10 +830,18 @@ describe('app_routing_effects', () => { dispatchSpy.and.callFake((action: Action) => { actualActions.push(action); }); - - effects.changeBrowserUrl$.subscribe(() => {}); } + let getResolvedPathSpy: jasmine.Spy; + + beforeEach(() => { + getResolvedPathSpy = spyOn(location, 'getResolvedPath') + .withArgs('/experiment/123') + .and.returnValue('/experiment/123') + .withArgs('/experiments') + .and.returnValue('/experiments'); + }); + it('navigates to default route if popstated to path without prefix', fakeAsync(() => { setAppRootAndSubscribe('/foo/bar/'); @@ -869,7 +916,7 @@ describe('app_routing_effects', () => { it('navigates with appRoot aware path when navRequest with relPath', fakeAsync(() => { setAppRootAndSubscribe('/foo/bar/'); - spyOn(location, 'getResolvedPath') + getResolvedPathSpy .withArgs('../experiment/123') .and.returnValue('/foo/bar/experiment/123'); @@ -896,27 +943,21 @@ describe('app_routing_effects', () => { describe('change url', () => { it('navigates to URL with path prefix prefixed', fakeAsync(() => { setAppRootAndSubscribe('/foo/bar/baz/'); - const activeRoute = buildRoute({ - routeKind: RouteKind.EXPERIMENTS, - pathname: '/experiments', - queryParams: [], - navigationOptions: { - replaceState: false, - }, - }); - store.overrideSelector(getActiveRoute, activeRoute); + + store.overrideSelector(getActiveRoute, null); store.refreshState(); getHashSpy.and.returnValue(''); getPathSpy.and.returnValue(''); getSearchSpy.and.returnValue([]); action.next( - actions.navigated({ - before: null, - after: activeRoute, + actions.navigationRequested({ + pathname: '/experiments', }) ); + tick(); + expect(pushStateSpy).toHaveBeenCalledWith('/foo/bar/baz/experiments'); })); });