Skip to content

fix(router): avoiding throw for app stability improvements #1344

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

Merged
merged 4 commits into from
Jul 27, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions nativescript-angular/router/ns-location-strategy.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Injectable } from "@angular/core";
import { LocationStrategy } from "@angular/common";
import { DefaultUrlSerializer, UrlSegmentGroup, UrlTree } from "@angular/router";
import { routerLog } from "../trace";
import { routerLog, routerError } from "../trace";
import { NavigationTransition, Frame } from "tns-core-modules/ui/frame";
import { isPresent } from "../lang-facade";
import { FrameService } from "../platform-providers";
Expand Down Expand Up @@ -286,10 +286,11 @@ export class NSLocationStrategy extends LocationStrategy {

// Methods for syncing with page navigation in PageRouterOutlet
public _beginBackPageNavigation(name: string, frame: Frame) {
routerLog("NSLocationStrategy.startGoBack()");
if (this._isPageNavigationBack) {
throw new Error("Calling startGoBack while going back.");
routerError("Attempted to call startGoBack while going back.");
return;
}
routerLog("NSLocationStrategy.startGoBack()");
this._isPageNavigationBack = true;

let { cachedFrame } = this.frameService.findFrame(frame);
Expand All @@ -302,10 +303,11 @@ export class NSLocationStrategy extends LocationStrategy {
}

public _finishBackPageNavigation() {
routerLog("NSLocationStrategy.finishBackPageNavigation()");
if (!this._isPageNavigationBack) {
throw new Error("Calling endGoBack while not going back.");
routerError("Attempted to call endGoBack while not going back.");
return;
}
routerLog("NSLocationStrategy.finishBackPageNavigation()");
this._isPageNavigationBack = false;
}

Expand All @@ -314,33 +316,35 @@ export class NSLocationStrategy extends LocationStrategy {
}

public _beginModalNavigation(frame: Frame): void {
routerLog("NSLocationStrategy._beginModalNavigation()");
routerLog("NSLocationStrategy._beginModalNavigation()");

let { cachedFrameRootOutlet } = this.frameService.findFrame(frame);
let { cachedFrameRootOutlet } = this.frameService.findFrame(frame);

const lastState = this.peekState(cachedFrameRootOutlet || this.currentOutlet);
const lastState = this.peekState(cachedFrameRootOutlet || this.currentOutlet);

if (lastState) {
lastState.isModalNavigation = true;
}
if (lastState) {
lastState.isModalNavigation = true;
}

this._isModalNavigation = true;
}
this._isModalNavigation = true;
}

public _beginCloseModalNavigation(): void {
routerLog("NSLocationStrategy.startCloseModal()");
if (this._isModalClosing) {
throw new Error("Calling startCloseModal while closing modal.");
routerError("Attempted to call startCloseModal while closing modal.");
return;
}
routerLog("NSLocationStrategy.startCloseModal()");
this._isModalClosing = true;
}

public _finishCloseModalNavigation() {
routerLog("NSLocationStrategy.finishCloseModalNavigation()");
if (!this._isModalClosing) {
throw new Error("Calling startCloseModal while not closing modal.");
routerError("Attempted to call startCloseModal while not closing modal.");
return;
}

routerLog("NSLocationStrategy.finishCloseModalNavigation()");
this._isModalNavigation = false;
this._isModalClosing = false;
}
Expand Down
20 changes: 14 additions & 6 deletions nativescript-angular/router/page-router-outlet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ function routeToString(activatedRoute: ActivatedRoute | ActivatedRouteSnapshot):
return activatedRoute.pathFromRoot.join("->");
}

const routeTransitionWarning = "This could be due to route transition timing and could be ignored.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vakrilov shoot so sorry this should be deleted as I don’t believe it ended up being used - not in front of computer but could fix up tomorrow or if you just want to remove this line just wanted to flag it before it went in.


@Directive({ selector: "page-router-outlet" }) // tslint:disable-line:directive-selector
export class PageRouterOutlet implements OnDestroy { // tslint:disable-line:directive-class-suffix
private activated: ComponentRef<any> | null = null;
Expand All @@ -126,14 +128,16 @@ export class PageRouterOutlet implements OnDestroy { // tslint:disable-line:dire

get component(): Object {
if (!this.activated) {
throw new Error("Outlet is not activated");
log("Outlet is not activated");
return;
}

return this.activated.instance;
}
get activatedRoute(): ActivatedRoute {
if (!this.activated) {
throw new Error("Outlet is not activated");
log("Outlet is not activated");
Copy link
Contributor Author

@NathanWalker NathanWalker Jun 29, 2018

Choose a reason for hiding this comment

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

@vakrilov worth noting that obviously this logic mirrors what Angular framework does for router-outlet: https://github.com/angular/angular/blob/master/packages/router/src/directives/router_outlet.ts#L76-L83 however again argument here is {N} is not a web app and therefore cannot withstand a throw in userland due to detrimental effect app crash has in mobile apps vs. web apps.

return;
}

return this._activatedRoute;
Expand Down Expand Up @@ -173,8 +177,9 @@ export class PageRouterOutlet implements OnDestroy { // tslint:disable-line:dire

deactivate(): void {
if (!this.locationStrategy._isPageNavigatingBack()) {
throw new Error("Currently not in page back navigation" +
" - component should be detached instead of deactivated.");
log("Currently not in page back navigation" +
" - component should be detached instead of deactivated." + routeTransitionWarning);
return;
}

log("PageRouterOutlet.deactivate() while going back - should destroy");
Expand All @@ -197,7 +202,8 @@ export class PageRouterOutlet implements OnDestroy { // tslint:disable-line:dire
*/
detach(): ComponentRef<any> {
if (!this.isActivated) {
throw new Error("Outlet is not activated");
log("Outlet is not activated");
return;
}

log("PageRouterOutlet.detach() - " + routeToString(this._activatedRoute));
Expand Down Expand Up @@ -232,7 +238,9 @@ export class PageRouterOutlet implements OnDestroy { // tslint:disable-line:dire
resolver: ComponentFactoryResolver | null): void {

if (this.locationStrategy._isPageNavigatingBack()) {
throw new Error("Currently in page back navigation - component should be reattached instead of activated.");
log("Currently in page back navigation - component should be reattached instead of activated. " +
routeTransitionWarning);
this.locationStrategy._finishBackPageNavigation();
}

log("PageRouterOutlet.activateWith() - " + routeToString(activatedRoute));
Expand Down
4 changes: 4 additions & 0 deletions nativescript-angular/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ export function routerLog(message: string): void {
write(message, routerTraceCategory);
}

export function routerError(message: string): void {
write(message, routerTraceCategory, messageType.error);
}

export function routeReuseStrategyLog(message: string): void {
write(message, routeReuseStrategyTraceCategory);
}
Expand Down