From ede9cb7c2f9069bea29cdd34d5deb89aa4434968 Mon Sep 17 00:00:00 2001 From: Jason Aden Date: Fri, 26 Jan 2018 16:10:22 -0800 Subject: [PATCH] Revert: "feat(router): add navigationSource and restoredState to NavigationStart event (#21728)" This reverts commit 3b7bab7d22064d545f1745dc3e062fcb1d91073b. Will be re-merged after fixing integration of minor breaking change. --- packages/common/src/location/location.ts | 10 ++- .../common/src/location/platform_location.ts | 5 +- packages/common/testing/src/location_mock.ts | 22 +++--- packages/platform-server/src/location.ts | 5 +- packages/router/src/events.ts | 47 ------------- packages/router/src/router.ts | 44 +++++------- packages/router/test/integration.spec.ts | 69 ------------------- tools/public_api_guard/common/common.d.ts | 6 +- tools/public_api_guard/common/testing.d.ts | 4 +- tools/public_api_guard/router/router.d.ts | 11 --- 10 files changed, 40 insertions(+), 183 deletions(-) diff --git a/packages/common/src/location/location.ts b/packages/common/src/location/location.ts index c09bce6258..d3997e4f90 100644 --- a/packages/common/src/location/location.ts +++ b/packages/common/src/location/location.ts @@ -14,7 +14,6 @@ import {LocationStrategy} from './location_strategy'; /** @experimental */ export interface PopStateEvent { pop?: boolean; - state?: any; type?: string; url?: string; } @@ -57,7 +56,6 @@ export class Location { this._subject.emit({ 'url': this.path(true), 'pop': true, - 'state': ev.state, 'type': ev.type, }); }); @@ -105,16 +103,16 @@ export class Location { * Changes the browsers URL to the normalized version of the given URL, and pushes a * new item onto the platform's history. */ - go(path: string, query: string = '', state: any = null): void { - this._platformStrategy.pushState(state, '', path, query); + go(path: string, query: string = ''): void { + this._platformStrategy.pushState(null, '', path, query); } /** * Changes the browsers URL to the normalized version of the given URL, and replaces * the top item on the platform's history stack. */ - replaceState(path: string, query: string = '', state: any = null): void { - this._platformStrategy.replaceState(state, '', path, query); + replaceState(path: string, query: string = ''): void { + this._platformStrategy.replaceState(null, '', path, query); } /** diff --git a/packages/common/src/location/platform_location.ts b/packages/common/src/location/platform_location.ts index 177b6a474a..0f5cb5f32e 100644 --- a/packages/common/src/location/platform_location.ts +++ b/packages/common/src/location/platform_location.ts @@ -58,10 +58,7 @@ export const LOCATION_INITIALIZED = new InjectionToken>('Location I * * @experimental */ -export interface LocationChangeEvent { - type: string; - state: any; -} +export interface LocationChangeEvent { type: string; } /** * @experimental diff --git a/packages/common/testing/src/location_mock.ts b/packages/common/testing/src/location_mock.ts index 802d9978f1..55b71a7e08 100644 --- a/packages/common/testing/src/location_mock.ts +++ b/packages/common/testing/src/location_mock.ts @@ -19,7 +19,7 @@ import {ISubscription} from 'rxjs/Subscription'; @Injectable() export class SpyLocation implements Location { urlChanges: string[] = []; - private _history: LocationState[] = [new LocationState('', '', null)]; + private _history: LocationState[] = [new LocationState('', '')]; private _historyIndex: number = 0; /** @internal */ _subject: EventEmitter = new EventEmitter(); @@ -34,8 +34,6 @@ export class SpyLocation implements Location { path(): string { return this._history[this._historyIndex].path; } - private state(): string { return this._history[this._historyIndex].state; } - isCurrentPathEqualTo(path: string, query: string = ''): boolean { const givenPath = path.endsWith('/') ? path.substring(0, path.length - 1) : path; const currPath = @@ -62,13 +60,13 @@ export class SpyLocation implements Location { return this._baseHref + url; } - go(path: string, query: string = '', state: any = null) { + go(path: string, query: string = '') { path = this.prepareExternalUrl(path); if (this._historyIndex > 0) { this._history.splice(this._historyIndex + 1); } - this._history.push(new LocationState(path, query, state)); + this._history.push(new LocationState(path, query)); this._historyIndex = this._history.length - 1; const locationState = this._history[this._historyIndex - 1]; @@ -81,7 +79,7 @@ export class SpyLocation implements Location { this._subject.emit({'url': url, 'pop': false}); } - replaceState(path: string, query: string = '', state: any = null) { + replaceState(path: string, query: string = '') { path = this.prepareExternalUrl(path); const history = this._history[this._historyIndex]; @@ -91,7 +89,6 @@ export class SpyLocation implements Location { history.path = path; history.query = query; - history.state = state; const url = path + (query.length > 0 ? ('?' + query) : ''); this.urlChanges.push('replace: ' + url); @@ -100,14 +97,14 @@ export class SpyLocation implements Location { forward() { if (this._historyIndex < (this._history.length - 1)) { this._historyIndex++; - this._subject.emit({'url': this.path(), 'state': this.state(), 'pop': true}); + this._subject.emit({'url': this.path(), 'pop': true}); } } back() { if (this._historyIndex > 0) { this._historyIndex--; - this._subject.emit({'url': this.path(), 'state': this.state(), 'pop': true}); + this._subject.emit({'url': this.path(), 'pop': true}); } } @@ -121,5 +118,10 @@ export class SpyLocation implements Location { } class LocationState { - constructor(public path: string, public query: string, public state: any) {} + path: string; + query: string; + constructor(path: string, query: string) { + this.path = path; + this.query = query; + } } diff --git a/packages/platform-server/src/location.ts b/packages/platform-server/src/location.ts index 34d1d71425..b4983799cb 100644 --- a/packages/platform-server/src/location.ts +++ b/packages/platform-server/src/location.ts @@ -63,9 +63,8 @@ export class ServerPlatformLocation implements PlatformLocation { } (this as{hash: string}).hash = value; const newUrl = this.url; - scheduleMicroTask(() => this._hashUpdate.next({ - type: 'hashchange', state: null, oldUrl, newUrl - } as LocationChangeEvent)); + scheduleMicroTask( + () => this._hashUpdate.next({ type: 'hashchange', oldUrl, newUrl } as LocationChangeEvent)); } replaceState(state: any, title: string, newUrl: string): void { diff --git a/packages/router/src/events.ts b/packages/router/src/events.ts index 67e1508499..b6a5d46fca 100644 --- a/packages/router/src/events.ts +++ b/packages/router/src/events.ts @@ -9,16 +9,6 @@ import {Route} from './config'; import {ActivatedRouteSnapshot, RouterStateSnapshot} from './router_state'; -/** - * @whatItDoes Identifies the trigger of the navigation. - * - * * 'imperative'--triggered by `router.navigateByUrl` or `router.navigate`. - * * 'popstate'--triggered by a popstate event - * * 'hashchange'--triggered by a hashchange event - * - * @experimental - */ -export type NavigationTrigger = 'imperative' | 'popstate' | 'hashchange'; /** * @whatItDoes Base for events the Router goes through, as opposed to events tied to a specific @@ -52,43 +42,6 @@ export class RouterEvent { * @stable */ export class NavigationStart extends RouterEvent { - /** - * Identifies the trigger of the navigation. - * - * * 'imperative'--triggered by `router.navigateByUrl` or `router.navigate`. - * * 'popstate'--triggered by a popstate event - * * 'hashchange'--triggered by a hashchange event - */ - navigationTrigger?: 'imperative'|'popstate'|'hashchange'; - - /** - * This contains the navigation id that pushed the history record that the router navigates - * back to. This is not null only when the navigation is triggered by a popstate event. - * - * The router assigns a navigationId to every router transition/navigation. Even when the user - * clicks on the back button in the browser, a new navigation id will be created. So from - * the perspective of the router, the router never "goes back". By using the `restoredState` - * and its navigationId, you can implement behavior that differentiates between creating new - * states - * and popstate events. In the latter case you can restore some remembered state (e.g., scroll - * position). - */ - restoredState?: {navigationId: number}|null; - - constructor( - /** @docsNotRequired */ - id: number, - /** @docsNotRequired */ - url: string, - /** @docsNotRequired */ - navigationTrigger: 'imperative'|'popstate'|'hashchange' = 'imperative', - /** @docsNotRequired */ - restoredState: {navigationId: number}|null = null) { - super(id, url); - this.navigationTrigger = navigationTrigger; - this.restoredState = restoredState; - } - /** @docsNotRequired */ toString(): string { return `NavigationStart(id: ${this.id}, url: '${this.url}')`; } } diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index fcbdeb7627..4c1f278c3c 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -21,7 +21,7 @@ import {applyRedirects} from './apply_redirects'; import {LoadedRouterConfig, QueryParamsHandling, Route, Routes, validateConfig} from './config'; import {createRouterState} from './create_router_state'; import {createUrlTree} from './create_url_tree'; -import {ActivationEnd, ChildActivationEnd, Event, GuardsCheckEnd, GuardsCheckStart, NavigationCancel, NavigationEnd, NavigationError, NavigationStart, NavigationTrigger, ResolveEnd, ResolveStart, RouteConfigLoadEnd, RouteConfigLoadStart, RoutesRecognized} from './events'; +import {ActivationEnd, ChildActivationEnd, Event, GuardsCheckEnd, GuardsCheckStart, NavigationCancel, NavigationEnd, NavigationError, NavigationStart, ResolveEnd, ResolveStart, RouteConfigLoadEnd, RouteConfigLoadStart, RoutesRecognized} from './events'; import {PreActivation} from './pre_activation'; import {recognize} from './recognize'; import {DefaultRouteReuseStrategy, DetachedRouteHandleInternal, RouteReuseStrategy} from './route_reuse_strategy'; @@ -164,6 +164,8 @@ function defaultErrorHandler(error: any): any { throw error; } +type NavigationSource = 'imperative' | 'popstate' | 'hashchange'; + type NavigationParams = { id: number, rawUrl: UrlTree, @@ -171,8 +173,7 @@ type NavigationParams = { resolve: any, reject: any, promise: Promise, - source: NavigationTrigger, - state: {navigationId: number} | null + source: NavigationSource, }; /** @@ -222,7 +223,6 @@ export class Router { * Indicates if at least one navigation happened. */ navigated: boolean = false; - private lastSuccessfulId: number = -1; /** * Used by RouterModule. This allows us to @@ -311,12 +311,8 @@ export class Router { if (!this.locationSubscription) { this.locationSubscription = this.location.subscribe(Zone.current.wrap((change: any) => { const rawUrlTree = this.urlSerializer.parse(change['url']); - const source: NavigationTrigger = change['type'] === 'popstate' ? 'popstate' : 'hashchange'; - const state = change.state && change.state.navigationId ? - {navigationId: change.state.navigationId} : - null; - setTimeout( - () => { this.scheduleNavigation(rawUrlTree, source, state, {replaceUrl: true}); }, 0); + const source: NavigationSource = change['type'] === 'popstate' ? 'popstate' : 'hashchange'; + setTimeout(() => { this.scheduleNavigation(rawUrlTree, source, {replaceUrl: true}); }, 0); })); } } @@ -345,7 +341,6 @@ export class Router { validateConfig(config); this.config = config; this.navigated = false; - this.lastSuccessfulId = -1; } /** @docsNotRequired */ @@ -454,7 +449,7 @@ export class Router { const urlTree = url instanceof UrlTree ? url : this.parseUrl(url); const mergedTree = this.urlHandlingStrategy.merge(urlTree, this.rawUrlTree); - return this.scheduleNavigation(mergedTree, 'imperative', null, extras); + return this.scheduleNavigation(mergedTree, 'imperative', extras); } /** @@ -527,9 +522,8 @@ export class Router { .subscribe(() => {}); } - private scheduleNavigation( - rawUrl: UrlTree, source: NavigationTrigger, state: {navigationId: number}|null, - extras: NavigationExtras): Promise { + private scheduleNavigation(rawUrl: UrlTree, source: NavigationSource, extras: NavigationExtras): + Promise { const lastNavigation = this.navigations.value; // If the user triggers a navigation imperatively (e.g., by using navigateByUrl), @@ -564,22 +558,21 @@ export class Router { }); const id = ++this.navigationId; - this.navigations.next({id, source, state, rawUrl, extras, resolve, reject, promise}); + this.navigations.next({id, source, rawUrl, extras, resolve, reject, promise}); // Make sure that the error is propagated even though `processNavigations` catch // handler does not rethrow return promise.catch((e: any) => Promise.reject(e)); } - private executeScheduledNavigation({id, rawUrl, extras, resolve, reject, source, - state}: NavigationParams): void { + private executeScheduledNavigation({id, rawUrl, extras, resolve, reject}: NavigationParams): + void { const url = this.urlHandlingStrategy.extract(rawUrl); const urlTransition = !this.navigated || url.toString() !== this.currentUrlTree.toString(); if ((this.onSameUrlNavigation === 'reload' ? true : urlTransition) && this.urlHandlingStrategy.shouldProcessUrl(rawUrl)) { - (this.events as Subject) - .next(new NavigationStart(id, this.serializeUrl(url), source, state)); + (this.events as Subject).next(new NavigationStart(id, this.serializeUrl(url))); Promise.resolve() .then( (_) => this.runNavigate( @@ -591,8 +584,7 @@ export class Router { } else if ( urlTransition && this.rawUrlTree && this.urlHandlingStrategy.shouldProcessUrl(this.rawUrlTree)) { - (this.events as Subject) - .next(new NavigationStart(id, this.serializeUrl(url), source, state)); + (this.events as Subject).next(new NavigationStart(id, this.serializeUrl(url))); Promise.resolve() .then( (_) => this.runNavigate( @@ -735,9 +727,9 @@ export class Router { if (!skipLocationChange) { const path = this.urlSerializer.serialize(this.rawUrlTree); if (this.location.isCurrentPathEqualTo(path) || replaceUrl) { - this.location.replaceState(path, '', {navigationId: id}); + this.location.replaceState(path); } else { - this.location.go(path, '', {navigationId: id}); + this.location.go(path); } } @@ -751,7 +743,6 @@ export class Router { () => { if (navigationIsSuccessful) { this.navigated = true; - this.lastSuccessfulId = id; (this.events as Subject) .next(new NavigationEnd( id, this.serializeUrl(url), this.serializeUrl(this.currentUrlTree))); @@ -793,8 +784,7 @@ export class Router { } private resetUrlToCurrentUrlTree(): void { - this.location.replaceState( - this.urlSerializer.serialize(this.rawUrlTree), '', {navigationId: this.lastSuccessfulId}); + this.location.replaceState(this.urlSerializer.serialize(this.rawUrlTree)); } } diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 9a09b4ba26..8df1c43ef8 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -76,28 +76,6 @@ describe('Integration', () => { ]); }))); - it('should set the restoredState to null when executing imperative navigations', - fakeAsync(inject([Router], (router: Router) => { - router.resetConfig([ - {path: '', component: SimpleCmp}, - {path: 'simple', component: SimpleCmp}, - ]); - - const fixture = createRoot(router, RootCmp); - let event: NavigationStart; - router.events.subscribe(e => { - if (e instanceof NavigationStart) { - event = e; - } - }); - - router.navigateByUrl('/simple'); - tick(); - - expect(event !.navigationTrigger).toEqual('imperative'); - expect(event !.restoredState).toEqual(null); - }))); - it('should not pollute browser history when replaceUrl is set to true', fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => { router.resetConfig([ @@ -488,34 +466,21 @@ describe('Integration', () => { [{path: 'simple', component: SimpleCmp}, {path: 'user/:name', component: UserCmp}] }]); - let event: NavigationStart; - router.events.subscribe(e => { - if (e instanceof NavigationStart) { - event = e; - } - }); router.navigateByUrl('/team/33/simple'); advance(fixture); expect(location.path()).toEqual('/team/33/simple'); - const simpleNavStart = event !; router.navigateByUrl('/team/22/user/victor'); advance(fixture); - const userVictorNavStart = event !; - location.back(); advance(fixture); expect(location.path()).toEqual('/team/33/simple'); - expect(event !.navigationTrigger).toEqual('hashchange'); - expect(event !.restoredState !.navigationId).toEqual(simpleNavStart.id); location.forward(); advance(fixture); expect(location.path()).toEqual('/team/22/user/victor'); - expect(event !.navigationTrigger).toEqual('hashchange'); - expect(event !.restoredState !.navigationId).toEqual(userVictorNavStart.id); }))); it('should navigate to the same url when config changes', @@ -903,40 +868,6 @@ describe('Integration', () => { expect(locationUrlBeforeEmittingError).toEqual('/simple'); })); - it('should reset the url with the right state when navigation errors', fakeAsync(() => { - const router: Router = TestBed.get(Router); - const location: SpyLocation = TestBed.get(Location); - const fixture = createRoot(router, RootCmp); - - router.resetConfig([ - {path: 'simple1', component: SimpleCmp}, {path: 'simple2', component: SimpleCmp}, - {path: 'throwing', component: ThrowingCmp} - ]); - - - let event: NavigationStart; - router.events.subscribe(e => { - if (e instanceof NavigationStart) { - event = e; - } - }); - - router.navigateByUrl('/simple1'); - advance(fixture); - const simple1NavStart = event !; - - router.navigateByUrl('/throwing').catch(() => null); - advance(fixture); - - router.navigateByUrl('/simple2'); - advance(fixture); - - location.back(); - tick(); - - expect(event !.restoredState !.navigationId).toEqual(simple1NavStart.id); - })); - it('should not trigger another navigation when resetting the url back due to a NavigationError', fakeAsync(() => { const router = TestBed.get(Router); diff --git a/tools/public_api_guard/common/common.d.ts b/tools/public_api_guard/common/common.d.ts index 7edceead7b..40d879e2ea 100644 --- a/tools/public_api_guard/common/common.d.ts +++ b/tools/public_api_guard/common/common.d.ts @@ -182,12 +182,12 @@ export declare class Location { constructor(platformStrategy: LocationStrategy); back(): void; forward(): void; - go(path: string, query?: string, state?: any): void; + go(path: string, query?: string): void; isCurrentPathEqualTo(path: string, query?: string): boolean; normalize(url: string): string; path(includeHash?: boolean): string; prepareExternalUrl(url: string): string; - replaceState(path: string, query?: string, state?: any): void; + replaceState(path: string, query?: string): void; subscribe(onNext: (value: PopStateEvent) => void, onThrow?: ((exception: any) => void) | null, onReturn?: (() => void) | null): ISubscription; static joinWithSlash(start: string, end: string): string; static normalizeQueryParams(params: string): string; @@ -199,7 +199,6 @@ export declare const LOCATION_INITIALIZED: InjectionToken>; /** @experimental */ export interface LocationChangeEvent { - state: any; type: string; } @@ -416,7 +415,6 @@ export declare enum Plural { /** @experimental */ export interface PopStateEvent { pop?: boolean; - state?: any; type?: string; url?: string; } diff --git a/tools/public_api_guard/common/testing.d.ts b/tools/public_api_guard/common/testing.d.ts index e428ab6692..edac756fc1 100644 --- a/tools/public_api_guard/common/testing.d.ts +++ b/tools/public_api_guard/common/testing.d.ts @@ -21,12 +21,12 @@ export declare class SpyLocation implements Location { urlChanges: string[]; back(): void; forward(): void; - go(path: string, query?: string, state?: any): void; + go(path: string, query?: string): void; isCurrentPathEqualTo(path: string, query?: string): boolean; normalize(url: string): string; path(): string; prepareExternalUrl(url: string): string; - replaceState(path: string, query?: string, state?: any): void; + replaceState(path: string, query?: string): void; setBaseHref(url: string): void; setInitialPath(url: string): void; simulateHashChange(pathname: string): void; diff --git a/tools/public_api_guard/router/router.d.ts b/tools/public_api_guard/router/router.d.ts index 7fa17a655e..f9f0524226 100644 --- a/tools/public_api_guard/router/router.d.ts +++ b/tools/public_api_guard/router/router.d.ts @@ -208,17 +208,6 @@ export interface NavigationExtras { /** @stable */ export declare class NavigationStart extends RouterEvent { - navigationTrigger?: 'imperative' | 'popstate' | 'hashchange'; - restoredState?: { - navigationId: number; - } | null; - constructor( - id: number, - url: string, - navigationTrigger?: 'imperative' | 'popstate' | 'hashchange', - restoredState?: { - navigationId: number; - } | null); toString(): string; }