bug(Router): Stack trace on unrecognized routes

[tiziano88]

@petebacondarwin @btford @IgorMinar

Code sample: http://plnkr.co/edit/Ly2gfBsyd1J4iJlTjA4C
To reproduce:

• http://run.plnkr.co/plunks/Ly2gfBsyd1J4iJlTjA4C/#/x1 (loads the child component correctly)
• in the same tab, change URL to http://run.plnkr.co/plunks/Ly2gfBsyd1J4iJlTjA4C/#/x2 (stack trace)

Note that navigating directly to the incorrect URL does not seem to trigger the issue; only navigating from a valid to an invalid route in the context of the same tab does.

Attempted fix: #7203

[petebacondarwin]

OK. This is a valid bug. We should throw a better error on an unmatched route in both cases: navigation from outside the app; navigation within the app.

[tiziano88]

Thanks @petebacondarwin ; is there any way at all to catch the current error? I was not able to find one, which means that this exception always escapes and, in our case, crashes the entire app. Unless a way exists to handle router errors in general, I think throwing a better error would not solve the issue in a meaningful way. Also, I would personally consider this at least a P1 issue, again under the assumption that there is currently no way of catching router errors. If such a way exists, then P2 is appropriate.

[petebacondarwin]

Well in the bigger picture of Angular 2 I am afraid that this is not going to be viewed as urgent - but I appreciate that it needs to be fixed.

So at the moment we have this bit of code:

this._locationSub = this._location.subscribe((change) => {
      // we call recognize ourselves
      this.recognize(change['url'])
          .then((instruction) => {
            this.navigateByInstruction(instruction, isPresent(change['pop']))
                .then((_) => {
                  // this is a popstate event; no need to change the URL
                  if (isPresent(change['pop']) && change['type'] != 'hashchange') {
                    return;
                  }
                  var emitPath = instruction.toUrlPath();
                  var emitQuery = instruction.toUrlQuery();
                  if (emitPath.length > 0 && emitPath[0] != '/') {
                    emitPath = '/' + emitPath;
                  }

                  // Because we've opted to use All hashchange events occur outside Angular.
                  // However, apps that are migrating might have hash links that operate outside
                  // angular to which routing must respond.
                  // To support these cases where we respond to hashchanges and redirect as a
                  // result, we need to replace the top item on the stack.
                  if (change['type'] == 'hashchange') {
                    if (instruction.toRootUrl() != this._location.path()) {
                      this._location.replaceState(emitPath, emitQuery);
                    }
                  } else {
                    this._location.go(emitPath, emitQuery);
                  }
                });
          });

The subscribe(onNext, onThrow, onReturn) method is only taking a single callback, onNext..
I think we should probably have an onThrow callback too but I am not sure what we should do with it.

Perhaps we should change the API of the router.subscribe(onNext) to also allow an onError callback, which we can call if a route change fails?

[tiziano88]

What I am suggesting is that for the time being, it would probably be better to just avoid throwing errors entirely (which is what I propose in #7203 ), rather than throwing unrecoverable ones. When a better mechanism exists for handling them, then a more meaningful error may be thrown instead.

[petebacondarwin]

I think I have a fix almost ready.

[petebacondarwin]

Here you go! I tried it with a local version of your Plunker and there is now no unwanted unhandled exception.

[tiziano88]

Awesome! Thanks for the fix, I'll drop my PR as soon as yours is merged then!

[naomiblack]

Closing because Pete's fix is merged!

[tiziano88]

Thanks for the fix! I have just tried syncing past it, but it does not seem to completely solve the issue in Dart; in particular, because in Dart it is only possible to subscribe to router updates with cancelOnError implicitly set to true, the router will stop getting updates after the first error.

[petebacondarwin]

@tiziano88 can you create a new issue to track this please

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}