Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Fix 592/1699 prevent route change start #2555

Conversation

sylvain-hamel
Copy link

Hi, this is my first PR ever so please let me know if did not do things the right way. I signed the CLA.

It's my attempt at fixing: #1699 and #592

This change adds support to calling preventDefault() on $route's $routeChangeStart event in order to cancel a change of route while leaving the browser's Url to its new location.

This is different from calling event.preventDefault() on $location's $locationChangeStart because that reverts the browser's Url to its last Url.

Basic usage

$rootScope.$on('$routeChangeStart', function(event, next, current) {
  event.preventDefault();
});

Example: Blocking a specific route change

Setup routes:

$routeProvider.when('/Book/new', { path: '/Book/new', templateUrl: 'partials/book.html', controller: 'BookControler'});
$routeProvider.when('/Book/:d',  { path: '/Book/:d', templateUrl: 'partials/book.html', controller: 'BookControler'});

As you can see, I added a path attribute to my routes in order to compare them when $routeChangeStart will be fired. You could instead give a name such as "New Book" and "Existing Book" to your routes and compare using names.

Cancel route:
In a controller, cancel route change when going from /Book/new to /Book/:d:

$rootScope.$on('$routeChangeStart', function(event, next, current) {
  if (current && current.path === "/Book/new" && next && next.path === "/Book/:d"){
    event.preventDefault();
  }
});

What do you think about this solution? Do you see any potential conflicts with other parts? Can you think of additional test cases I should write?

@petebacondarwin
Copy link
Contributor

Hi @redhotsly
Thanks for making this pull request.

You are basically trying to solve the same problem as was rejected in a previous PR: #2398 (comment). The router is not designed to become out of sync with the URL and providing this functionality could well cause that.

Closing for now.

@sylvain-hamel
Copy link
Author

Hi @petebacondarwin,
Thanks for reviewing my PR. I'm indeed trying to solve the same issue. I read your comments on PR #2398. Can we talk/chat about this? There has been many +1 on those issues. I think it's worth taking the time to think about a good solution.
Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants