bug(Router): Stack trace on unrecognized routes #7349

Activity

1. petebacondarwin commented on Mar 2, 2016

   

   petebacondarwin

   on Mar 2, 2016

   Contributor

   More actions

   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.
2. petebacondarwin

   added

   type: bug/fix

   comp: router

   P2: required

   on Mar 2, 2016
3. petebacondarwin

   self-assigned this

   on Mar 2, 2016
4. tiziano88 commented on Mar 2, 2016

   

   tiziano88

   on Mar 2, 2016

   Author

   More actions

   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.
5. petebacondarwin commented on Mar 2, 2016

   

   petebacondarwin

   on Mar 2, 2016

   Contributor

   More actions

   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?
6. tiziano88 commented on Mar 2, 2016

   

   tiziano88

   on Mar 2, 2016

   Author

   More actions

   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.
7. petebacondarwin commented on Mar 2, 2016

   

   petebacondarwin

   on Mar 2, 2016

   Contributor

   More actions

   I think I have a fix almost ready.
8. petebacondarwin commented on Mar 2, 2016

   

   petebacondarwin

   on Mar 2, 2016

   Contributor

   More actions

   Here you go! I tried it with a local version of your Plunker and there is now no unwanted unhandled exception.
9. tiziano88 commented on Mar 4, 2016

   

   tiziano88

   on Mar 4, 2016

   Author

   More actions

   Awesome! Thanks for the fix, I'll drop my PR as soon as yours is merged then!
10. naomiblack commented on Mar 7, 2016

    

    naomiblack

    on Mar 7, 2016

    Contributor

    More actions

    Closing because Pete's fix is merged!
11. naomiblack

    closed this as completedon Mar 7, 2016
12. alexeagle

    added a commit that references this issue on Mar 10, 2016

    fix(router): handle URL that does not match a route

    ...

    This issue will close once commit 8e3e450 is merged into the 'main' branch.

    8e3e450
13. tiziano88 commented on Mar 10, 2016

    

    tiziano88

    on Mar 10, 2016

    Author

    More actions

    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.
14. petebacondarwin commented on Mar 10, 2016

    

    petebacondarwin

    on Mar 10, 2016

    Contributor

    More actions

    @tiziano88 can you create a new issue to track this please
15. angular-automatic-lock-bot commented on Sep 8, 2019

    

    angular-automatic-lock-botBot

    on Sep 8, 2019

    More actions

    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.}
16. angular-automatic-lock-bot

    locked and limited conversation to collaborators on Sep 8, 2019