Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Router navigate to current url #19463

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/common/testing/src/location_mock.ts
Expand Up @@ -41,7 +41,9 @@ export class SpyLocation implements Location {
return currPath == givenPath + (query.length > 0 ? ('?' + query) : '');
}

simulateUrlPop(pathname: string) { this._subject.emit({'url': pathname, 'pop': true}); }
simulateUrlPop(pathname: string) {
this._subject.emit({'url': pathname, 'pop': true, 'type': 'popstate'});
}

simulateHashChange(pathname: string) {
// Because we don't prevent the native event, the browser will independently update the path
Expand Down
170 changes: 92 additions & 78 deletions packages/router/src/router.ts
Expand Up @@ -43,12 +43,12 @@ declare let Zone: any;
*/
export interface NavigationExtras {
/**
* Enables relative navigation from the current ActivatedRoute.
*
* Configuration:
*
* ```
* [{
* Enables relative navigation from the current ActivatedRoute.
*
* Configuration:
*
* ```
* [{
* path: 'parent',
* component: ParentComponent,
* children: [{
Expand All @@ -59,92 +59,92 @@ export interface NavigationExtras {
* component: ChildComponent
* }]
* }]
* ```
*
* Navigate to list route from child route:
*
* ```
* @Component({...})
* class ChildComponent {
* ```
*
* Navigate to list route from child route:
*
* ```
* @Component({...})
* class ChildComponent {
* constructor(private router: Router, private route: ActivatedRoute) {}
*
* go() {
* this.router.navigate(['../list'], { relativeTo: this.route });
* }
* }
* ```
*/
* ```
*/
relativeTo?: ActivatedRoute|null;

/**
* Sets query parameters to the URL.
*
* ```
* // Navigate to /results?page=1
* this.router.navigate(['/results'], { queryParams: { page: 1 } });
* ```
*/
* Sets query parameters to the URL.
*
* ```
* // Navigate to /results?page=1
* this.router.navigate(['/results'], { queryParams: { page: 1 } });
* ```
*/
queryParams?: Params|null;

/**
* Sets the hash fragment for the URL.
*
* ```
* // Navigate to /results#top
* this.router.navigate(['/results'], { fragment: 'top' });
* ```
*/
* Sets the hash fragment for the URL.
*
* ```
* // Navigate to /results#top
* this.router.navigate(['/results'], { fragment: 'top' });
* ```
*/
fragment?: string;

/**
* Preserves the query parameters for the next navigation.
*
* deprecated, use `queryParamsHandling` instead
*
* ```
* // Preserve query params from /results?page=1 to /view?page=1
* this.router.navigate(['/view'], { preserveQueryParams: true });
* ```
*
* @deprecated since v4
*/
* Preserves the query parameters for the next navigation.
*
* deprecated, use `queryParamsHandling` instead
*
* ```
* // Preserve query params from /results?page=1 to /view?page=1
* this.router.navigate(['/view'], { preserveQueryParams: true });
* ```
*
* @deprecated since v4
*/
preserveQueryParams?: boolean;

/**
* config strategy to handle the query parameters for the next navigation.
*
* ```
* // from /results?page=1 to /view?page=1&page=2
* this.router.navigate(['/view'], { queryParams: { page: 2 }, queryParamsHandling: "merge" });
* ```
*/
* config strategy to handle the query parameters for the next navigation.
*
* ```
* // from /results?page=1 to /view?page=1&page=2
* this.router.navigate(['/view'], { queryParams: { page: 2 }, queryParamsHandling: "merge" });
* ```
*/
queryParamsHandling?: QueryParamsHandling|null;
/**
* Preserves the fragment for the next navigation
*
* ```
* // Preserve fragment from /results#top to /view#top
* this.router.navigate(['/view'], { preserveFragment: true });
* ```
*/
* Preserves the fragment for the next navigation
*
* ```
* // Preserve fragment from /results#top to /view#top
* this.router.navigate(['/view'], { preserveFragment: true });
* ```
*/
preserveFragment?: boolean;
/**
* Navigates without pushing a new state into history.
*
* ```
* // Navigate silently to /view
* this.router.navigate(['/view'], { skipLocationChange: true });
* ```
*/
* Navigates without pushing a new state into history.
*
* ```
* // Navigate silently to /view
* this.router.navigate(['/view'], { skipLocationChange: true });
* ```
*/
skipLocationChange?: boolean;
/**
* Navigates while replacing the current state in history.
*
* ```
* // Navigate to /view
* this.router.navigate(['/view'], { replaceUrl: true });
* ```
*/
* Navigates while replacing the current state in history.
*
* ```
* // Navigate to /view
* this.router.navigate(['/view'], { replaceUrl: true });
* ```
*/
replaceUrl?: boolean;
}

Expand Down Expand Up @@ -241,6 +241,14 @@ export class Router {

routeReuseStrategy: RouteReuseStrategy = new DefaultRouteReuseStrategy();

/**
* Define what the router should do if it receives a navigation request to the current URL.
* By default, the router will ignore this navigation. However, this prevents features such
* as a "refresh" button. Use this option to configure the behavior when navigating to the
* current URL. Default is 'ignore'.
*/
onSameUrlNavigation: 'reload'|'ignore' = 'ignore';

/**
* Creates the router service.
*/
Expand Down Expand Up @@ -518,11 +526,18 @@ export class Router {

// Because of a bug in IE and Edge, the location class fires two events (popstate and
// hashchange) every single time. The second one should be ignored. Otherwise, the URL will
// flicker.
// flicker. Handles the case when a popstate was emitted first.
if (lastNavigation && source == 'hashchange' && lastNavigation.source === 'popstate' &&
lastNavigation.rawUrl.toString() === rawUrl.toString()) {
return Promise.resolve(true); // return value is not used
}
// Because of a bug in IE and Edge, the location class fires two events (popstate and
// hashchange) every single time. The second one should be ignored. Otherwise, the URL will
// flicker. Handles the case when a hashchange was emitted first.
if (lastNavigation && source == 'popstate' && lastNavigation.source === 'hashchange' &&
lastNavigation.rawUrl.toString() === rawUrl.toString()) {
return Promise.resolve(true); // return value is not used
}

let resolve: any = null;
let reject: any = null;
Expand All @@ -545,7 +560,8 @@ export class Router {
const url = this.urlHandlingStrategy.extract(rawUrl);
const urlTransition = !this.navigated || url.toString() !== this.currentUrlTree.toString();

if (urlTransition && this.urlHandlingStrategy.shouldProcessUrl(rawUrl)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this not a breaking change? It looks like the correct logic based on what you're fixing here, but it also seems like this could change behavior in existing apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, that's true about any bug fix.

So I think the question here is: can we find a real example of a working application that won't work with this change?

And I think we cannot find one, and here is why:

  • Guards and resolvers are side-effect free (or at least idempotent).
  • This change will only make a difference when navigating to the same URL. The same URL means the same RouterStateSnapshot, which means that all params will be the same, all data will be the same => no activated routes will be updated, no components will be removed or added.
  • So the only difference is that the router will dispatch a group of events corresponding to the navigation.

Of course, it's possible to come up with an example where an app would behave differently. Since resolvers will rerun when navigating to the same url, you may get fresh data. But I'd argue that this is very unlikely to break anyone.

Copy link
Contributor

@jasonaden jasonaden Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on most of that logic. My only concern is urlTransition will always be true on the first navigation. For subsequent navigations, I think the new logic is fine.

So we could try this on g3 as-is, or we could change the logic to:

if (!this.navigated && this.urlHandlingStrategy.shouldProcessUrl(rawUrl)) {

Thoughts?

if ((this.onSameUrlNavigation === 'reload' ? true : urlTransition) &&
this.urlHandlingStrategy.shouldProcessUrl(rawUrl)) {
(this.events as Subject<Event>).next(new NavigationStart(id, this.serializeUrl(url)));
Promise.resolve()
.then(
Expand Down Expand Up @@ -573,10 +589,9 @@ export class Router {
}

private runNavigate(
url: UrlTree, rawUrl: UrlTree, shouldPreventPushState: boolean, shouldReplaceUrl: boolean,
id: number, precreatedState: RouterStateSnapshot|null): Promise<boolean> {
url: UrlTree, rawUrl: UrlTree, skipLocationChange: boolean, replaceUrl: boolean, id: number,
precreatedState: RouterStateSnapshot|null): Promise<boolean> {
if (id !== this.navigationId) {
this.location.go(this.urlSerializer.serialize(this.currentUrlTree));
(this.events as Subject<Event>)
.next(new NavigationCancel(
id, this.serializeUrl(url),
Expand Down Expand Up @@ -698,9 +713,9 @@ export class Router {

(this as{routerState: RouterState}).routerState = state;

if (!shouldPreventPushState) {
if (!skipLocationChange) {
const path = this.urlSerializer.serialize(this.rawUrlTree);
if (this.location.isCurrentPathEqualTo(path) || shouldReplaceUrl) {
if (this.location.isCurrentPathEqualTo(path) || replaceUrl) {
this.location.replaceState(path);
} else {
this.location.go(path);
Expand Down Expand Up @@ -748,14 +763,13 @@ export class Router {
(this as{routerState: RouterState}).routerState = storedState;
this.currentUrlTree = storedUrl;
this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, rawUrl);
this.location.replaceState(this.serializeUrl(this.rawUrlTree));
this.resetUrlToCurrentUrlTree();
});
});
}

private resetUrlToCurrentUrlTree(): void {
const path = this.urlSerializer.serialize(this.rawUrlTree);
this.location.replaceState(path);
this.location.replaceState(this.urlSerializer.serialize(this.rawUrlTree));
}
}

Expand Down
17 changes: 16 additions & 1 deletion packages/router/src/router_module.ts
Expand Up @@ -129,12 +129,15 @@ export class RouterModule {
* Creates a module with all the router providers and directives. It also optionally sets up an
* application listener to perform an initial navigation.
*
* Options:
* Options (see {@link ExtraOptions}):
* * `enableTracing` makes the router log all its internal events to the console.
* * `useHash` enables the location strategy that uses the URL fragment instead of the history
* API.
* * `initialNavigation` disables the initial navigation.
* * `errorHandler` provides a custom error handler.
* * `preloadingStrategy` configures a preloading strategy (see {@link PreloadAllModules}).
* * `onSameUrlNavigation` configures how the router handles navigation to the current URL. See
* {@link ExtraOptions} for more details.
*/
static forRoot(routes: Routes, config?: ExtraOptions): ModuleWithProviders {
return {
Expand Down Expand Up @@ -267,6 +270,14 @@ export interface ExtraOptions {
* Configures a preloading strategy. See {@link PreloadAllModules}.
*/
preloadingStrategy?: any;

/**
* Define what the router should do if it receives a navigation request to the current URL.
* By default, the router will ignore this navigation. However, this prevents features such
* as a "refresh" button. Use this option to configure the behavior when navigating to the
* current URL. Default is 'ignore'.
*/
onSameUrlNavigation?: 'reload'|'ignore';
}

export function setupRouter(
Expand Down Expand Up @@ -299,6 +310,10 @@ export function setupRouter(
});
}

if (opts.onSameUrlNavigation) {
router.onSameUrlNavigation = opts.onSameUrlNavigation;
}

return router;
}

Expand Down