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

NathanWalker · 2018-05-22T18:14:55Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/nativescript-angular/blob/master/DevelopmentWorkflow.md#running-the-tests
Tests for the changes are included.

What is the current behavior?

During normal usage of a NativeScript app a user can experience instability and unresponsiveness while navigating around an app. This is often experienced when monkeying around as users enjoy doing with mobile apps. Due to extraneous usage of throw throughout router handling, this monkeying around of a {N} app would eventually cause one of these throw's to trigger and throwing the app into unstable state and other times just simply crash.

What is the new behavior?

Developers can now properly commence a deep dive debugging sessions by turning tracing on to investigate various UX flows to find suitable fixes within their own apps.

NathanWalker · 2018-05-24T18:47:40Z

I should mention that an alternative to throw would be to introduce dev vs. prod modes much like Angular's enableProdMode which would avoid throw in production but rather trace there to avoid end user crashes. This would allow devs to debug certain ux flows in development without fear that a missed flow would crash an end user - race conditions are the most likely culprits which are nearly impossible to test reliably during development. Users forgive instability by installing an app update. They are far less forgiving about crashes. Most often you will get immediately removed from users phone if that happens.

Mobile app development is uniquely different than web development in this regard. Users are used to refreshing a broken website. Force quitting and relaunching an app however due to crash is more likely to result in complete removal of app.

/cc @vakrilov @sis0k0 @hdeshev

vakrilov · 2018-05-30T15:19:38Z

Hey @NathanWalker
I think the idea of production Mode or and extended tracing mechanic is the better approach here.

The problem here is that event if those throws are not there - the user will end up navigating to a blank page which is nearly the same as crash. Simply console.log will leave developers unable to handle this cases in production.

Better approach - we can work on extending the tracing module, so that even if we don't throw in certain cases that we know something bad has happened - we can provide an way for developers to handle and report the error to analytics/crash-reporting for their production apps.

Let me know if this makes sense and I can open an issue (probably in core repo) and we can work out the API there.

NathanWalker · 2018-06-01T17:58:25Z

@vakrilov I like your idea of extending the tracing module. Main thing I would encourage is to try and help prevent production {N} apps from crashing caused by a throw.
In development it could throw to provide developer a chance to see it clearly and handle however in production the path would instead allow that code path to go through tracing module (and not throw) to prevent a production crash (even if app is unstable at that point - the argument is that instability in these [most often more rare] race conditions flows is better than a crash with mobile apps).

Ultimately control over throw vs. trace would be most valuable long-term solution I think. Perhaps as a bootstrap flag/option - that way developers have the control they need to handle things in best way for their given app/situation and per their own discretion.

NathanWalker · 2018-06-18T15:06:44Z

Closing this in favor of this strategy.

NathanWalker · 2018-06-29T16:36:54Z

@vakrilov Reopening since this strategy is not available yet and this is currently causing production crashes. More than anything we should discuss this since routing is the most fragile portion of any app due to user navigating erratically. There should at minimum be 0 throw's in any code related to routing but instead be all traced upon developer's discretion.

NathanWalker · 2018-06-29T17:04:09Z

nativescript-angular/router/page-router-outlet.ts

        }

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


@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.

NathanWalker · 2018-06-29T17:07:27Z

nativescript-angular/router/page-router-outlet.ts

@@ -229,7 +235,8 @@ 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);
+            return;


@vakrilov This is actually the biggest culprit to Android crashes and app becoming unresponsive of them all. On Android only (very rarely if ever happens on iOS) sometimes the back page navigation transition timing is different and this could be called before _finishBackPageNavigation gets fired on location strategy however just logging and allowing it to finish will correct itself on it's own. Definitely don't want to throw here.

NathanWalker · 2018-06-29T17:15:07Z

Also issues tying back to this throw have been reported in several different forms in the past:
#1115
#1151
Comments point to different resolutions however root cause is all tied back to the throw in here. There are likely many others that have run into this without a proper resolution to fix and did not bother to report since it is in fact quite confusing and problematic.

NathanWalker · 2018-06-29T17:16:53Z

It's also possible some improvements found here: #1404 could improve the situation however again throw should be avoided around routing at all costs.

NathanWalker · 2018-06-29T18:58:21Z

@vakrilov would be curious your thoughts around:

activateWith(
        activatedRoute: ActivatedRoute,
        resolver: ComponentFactoryResolver | null): void {

        if (this.locationStrategy._isPageNavigatingBack()) {

and how best to handle when _isPageNavigationBack is still true there. Appears transient and rather tricky to solve. Perhaps you have some insight into why that happens and how best to gracefully handle for the app rather than throw there? Appears simply ignoring is not best way to go but we need something to handle this problematic case. Again this can be repro'd in the PNP repo (ping privately for details).

NathanWalker · 2018-06-29T19:17:55Z

Ok upon further testing what I have in this PR now solves it 100%.
The solution is rather simple and avoids all strange app behavior that could previously occur:
https://github.com/NativeScript/nativescript-angular/pull/1344/files#diff-5450fcefd2730d6a0ba845ec1f521599R239
Simply flagging that off and finishing at that point allows app to behave as normal in this condition.

I can repro this privately to any core team member via screenshare to anyone that would like to explore further. We are using this PR in a custom pack in PNP now to resolve this issue with 100% reliability.

lambourn · 2018-07-04T10:49:59Z

100% agree on @NathanWalker 's approach here to ultimately not throw inside routing related functionality.

In our case (using named page-router-outlets after migrating to {N} 4.1 and angular 6.0.6) navigation is completely broken now due to the throw in page-router-outlet

vakrilov · 2018-07-13T12:04:06Z

Hey @NathanWalker - 👍 👍 👍 for the PR
I agree that the router failures should not cause crashes. Your changes are definitely a step in the right direction.

We had some router-related PRs and testing during the last 2 weeks and that's why this PR was pushed back a little in the pipeline. Can you please rebase it so that we can put it trough the CI again?

One more thing. There is a good change that if you reach the throw (now log) paths, something bad happened in the navigation and you might even not be able to navigate again. With this PR you would only see the log message is you have enabled the ns-router trace category. If you haven't done that, you might end up in a situation that your navigation stopped working, but you don't see any useful errors and it might not be obvious that you should enable the ns-router logging to get those.

Can you change those cases to use trace.write() with error type. That way the messages will show up in the console even if you haven't specifically enabled ns-router category. We have helper methods defined for some other trace categories, for example renderer - you can use the same approach for the router. As the type is error you can get rid of the "Warn:" prefix in the messages.

NathanWalker · 2018-07-15T22:35:30Z

@vakrilov sounds good, I made the changes you requested 👍

ADjenkov · 2018-07-16T12:32:55Z

nativescript-angular/router/ns-location-strategy.ts

-        let { cachedFrameRootOutlet } = this.frameService.findFrame(frame);
-
-        const lastState = this.peekState(cachedFrameRootOutlet || this.currentOutlet);
+    public _beginModalNavigation(): void {


Shouldn't remove the frame: Frame param. Also, some logic from the method is removed. I guess something gone went while updating the branch?

ADjenkov · 2018-07-16T13:32:41Z

nativescript-angular/router/ns-location-strategy.ts

-        }
-
-        const lastState = this.peekState(this.currentOutlet);
+    public _beginPageNavigation(name: string): NavigationOptions {


Same as above - the entire method logic is changed.

vakrilov · 2018-07-23T07:31:24Z

Hey @NathanWalker - there are some build errors that @ADjenkov commented on. Probably a result of a the rebase.

NathanWalker · 2018-07-26T06:28:46Z

I’ll take a look on Thursday definitely sounds like rebase issue.

NathanWalker · 2018-07-26T18:51:01Z

@vakrilov should be fixed up now - Good eye @ADjenkov thanks for catching that!

vakrilov · 2018-07-27T05:42:11Z

test

NathanWalker · 2018-07-27T06:18:12Z

nativescript-angular/router/page-router-outlet.ts

@@ -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.";


@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.

ghost added the ♥ community PR label May 22, 2018

NathanWalker force-pushed the fix-routing-throw branch from 8038a53 to 610a394 Compare May 24, 2018 17:12

NathanWalker force-pushed the fix-routing-throw branch from 610a394 to 89c1158 Compare May 30, 2018 03:09

NathanWalker force-pushed the fix-routing-throw branch 2 times, most recently from d6297fb to 26ac24c Compare June 1, 2018 01:03

NathanWalker closed this Jun 18, 2018

ghost removed the ♥ community PR label Jun 18, 2018

NathanWalker reopened this Jun 29, 2018

ghost added the ♥ community PR label Jun 29, 2018

NathanWalker force-pushed the fix-routing-throw branch 3 times, most recently from 2d96bb5 to 88d5725 Compare June 29, 2018 17:02

NathanWalker commented Jun 29, 2018

View reviewed changes

NathanWalker force-pushed the fix-routing-throw branch from 88d5725 to b3e136a Compare June 29, 2018 19:14

NathanWalker force-pushed the fix-routing-throw branch 2 times, most recently from c02d468 to 1b5bfae Compare July 15, 2018 22:32

ADjenkov self-requested a review July 16, 2018 08:41

ADjenkov approved these changes Jul 16, 2018

View reviewed changes

ghost assigned ADjenkov Jul 16, 2018

ghost added in progress and removed ♥ community PR labels Jul 16, 2018

ADjenkov suggested changes Jul 16, 2018

View reviewed changes

ADjenkov reviewed Jul 16, 2018

View reviewed changes

NathanWalker force-pushed the fix-routing-throw branch from e21c068 to 31d31f2 Compare July 26, 2018 18:07

docs: update issue template

4ab6c2f

NathanWalker force-pushed the fix-routing-throw branch from 31d31f2 to 4ab6c2f Compare July 26, 2018 18:49

chore: fix tslint

31ad990

ghost assigned vakrilov Jul 27, 2018

chore: fix tslint - again

971d05c

NathanWalker commented Jul 27, 2018

View reviewed changes

chore: remove routeTransitionWarning

9548fd9

vakrilov approved these changes Jul 27, 2018

View reviewed changes

vakrilov merged commit 82747df into NativeScript:master Jul 27, 2018

ghost removed the in progress label Jul 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NathanWalker commented May 22, 2018 •

edited

Loading

NathanWalker commented May 24, 2018 •

edited

Loading

vakrilov commented May 30, 2018

NathanWalker commented Jun 1, 2018

NathanWalker commented Jun 18, 2018

NathanWalker commented Jun 29, 2018

NathanWalker Jun 29, 2018 •

edited

Loading

NathanWalker Jun 29, 2018 •

edited

Loading

NathanWalker commented Jun 29, 2018 •

edited

Loading

NathanWalker commented Jun 29, 2018 •

edited

Loading

NathanWalker commented Jun 29, 2018

NathanWalker commented Jun 29, 2018

lambourn commented Jul 4, 2018 •

edited

Loading

vakrilov commented Jul 13, 2018

NathanWalker commented Jul 15, 2018

ADjenkov Jul 16, 2018

ADjenkov Jul 16, 2018

vakrilov commented Jul 23, 2018

NathanWalker commented Jul 26, 2018

NathanWalker commented Jul 26, 2018

vakrilov commented Jul 27, 2018

NathanWalker Jul 27, 2018

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

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

Conversation

NathanWalker commented May 22, 2018 • edited Loading

PR Checklist

What is the current behavior?

What is the new behavior?

NathanWalker commented May 24, 2018 • edited Loading

vakrilov commented May 30, 2018

NathanWalker commented Jun 1, 2018

NathanWalker commented Jun 18, 2018

NathanWalker commented Jun 29, 2018

NathanWalker Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

NathanWalker Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

NathanWalker commented Jun 29, 2018 • edited Loading

NathanWalker commented Jun 29, 2018 • edited Loading

NathanWalker commented Jun 29, 2018

NathanWalker commented Jun 29, 2018

lambourn commented Jul 4, 2018 • edited Loading

vakrilov commented Jul 13, 2018

NathanWalker commented Jul 15, 2018

ADjenkov Jul 16, 2018

Choose a reason for hiding this comment

ADjenkov Jul 16, 2018

Choose a reason for hiding this comment

vakrilov commented Jul 23, 2018

NathanWalker commented Jul 26, 2018

NathanWalker commented Jul 26, 2018

vakrilov commented Jul 27, 2018

NathanWalker Jul 27, 2018

Choose a reason for hiding this comment

NathanWalker commented May 22, 2018 •

edited

Loading

NathanWalker commented May 24, 2018 •

edited

Loading

NathanWalker Jun 29, 2018 •

edited

Loading

NathanWalker Jun 29, 2018 •

edited

Loading

NathanWalker commented Jun 29, 2018 •

edited

Loading

NathanWalker commented Jun 29, 2018 •

edited

Loading

lambourn commented Jul 4, 2018 •

edited

Loading