feat($route): add support for the `reloadOnUrl` configuration option #15002

gkalpak · 2016-08-07T16:54:38Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature.

What is the current behavior? (You can also link to an open issue here)
When the path changes and the new URL maps to the current route, the route is reloaded.
See #14999 (and some of the issues mentioned there).

What is the new behavior (if this is a feature change)?
The user can determine that specific routes should not get reloaded, when the path changes and the new URL maps to the current route.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:
Enables users to specify that a particular route should not be reloaded after a URL change
(including a change in $location.path()), if the new URL maps to the same route.
The default behavior is still to always load the matched route when any part of the URL changes.

Related to #1699, #5860, #14999 (potentially closing the first two).

Fixes #7925.

gkalpak · 2016-08-07T17:00:33Z

I am not super happy with the API (reloadOnUrl is a little cryptic - as is reloadOnSearch). I would prefer something like:

reloadOnChange: {
  hash: true/false,
  search: true/false,
  url: true/false
}

But at this point I figured it is better to (a) keep consistent with the existing API and (b) not introduce multiple way to do the same thing. But we might want to consider deprecating the existing way and introducing a different API in a future version.

gkalpak · 2016-08-07T17:06:38Z

Btw, in the future we might want to make this more flexible by allowing the user to specify a function (or service) for determining whether to reload or not. This would allow to have a default value for the route, but overwrite it in specific cases.

Mischi · 2016-08-16T19:47:01Z

Both features where the original reason for @davidreher and me to choose the now deprecated ngComponentRouter. We would love to see see these features added to ngRoute!

gkalpak · 2016-08-17T13:14:29Z

Also, considering that Angular 2's latest router re-uses components by default (afaik), it might be a good idea to re-use components by default as well (for 1.6 or 1.7).

Narretz · 2016-11-29T18:31:31Z

So this PR is basically a sub-set of the old problem where you wan't to change the location but not let Angular do anything (like change / reload a route).
With it, you can now "change the location to something that would map to the same route (but with different route params)"
The problem I see is that a static route definition doesn't really make the following use cases simpler (#1699, #3789):

So our route is like /thing/:id. When creating a new thing they go to /thing/new. Once the thing has been successfully saved we want to change the route to /thing/1234

Or when someone enters the wrong title for a particular ID. See Stackoverflow, when you change the title but enter just the ID, it will correct the title portion of the URL to be correct.

If the route never reloads on url change, if you do /thing/5678, then it also doesn't change the route, and you'd have to manually do work in the routeUpdate event (e.g. fetching data), instead of relying on resolves.

So I think we either need a $location function that doesn't trigger route change (don't really like that, though) or a dynamic way to decide if a route should be reloaded.

Another idea is to keep the template / DOM and re-run the resolves, and push the new data to the scope, so any components could handle data changes via onChanges or similar. This might be too much for vanilla ngRoute though.

gkalpak · 2016-11-29T19:43:53Z

The problem I see is that a static route definition doesn't really make the following use cases simpler (#1699, #3789)

This doesn't indeed cover #3789 (in a convenient way), but that sounds better handled with a redirection tbh.

Regarding #1699, I think this PR covers their exact usecase: You are on /thing/new, you create and save a new item (so at that point you have all the data you need for /thing/123), you change the URL to /thing/123 and the route doesn't get reloaded (including the resolves). If you need to also re-resolve the data, then you can handle that inside the controller (which by some is considered a better practice anyway).

--
WRT dynamically deciding whether a route should be reloaded or not:

Having the controller/component decide is not a good idea imo. This is something that needs to be decided on a route level, so should be in Route Definition Object.
Adding support for this by allowing a function or service as value of reloadOnSearch sounds like an interesting idea. It is still configurable on a per-route basis, but can be dynamic and the controller can have a saying via some DI'ed service.

Taking (2) one step further, we could introduce a new reloadStrategy property that accepts a service that implements the ReloadStrategy interface. This could replace both reloadOnSearch and reloadOnUrl. We could for example have built-in strategies for not reloading when the hash/search/path changes, but users could specify their own.

--

So I think we either need a $location function that doesn't trigger route change

In general, I believe $location should be kept out of this (unless we are talking about a url change that should not update $location's internal path/search/hash properties/state, which would be asking for serious trouble and abuse - but is a different discussion). If we are talking about ngRoute-specific behavior/features, it should be entriely included in ngRoute.

miwillhite · 2017-11-03T18:07:31Z

Any updates on this?

Narretz · 2017-11-04T13:51:48Z

@miwillhite No, not at the moment.
To get some traction for this, you could test the patch and see if it would work for your use-case, or otherwise give some feedback on the behavior / implementation.

Btw, does ui-router have this functionality?

Narretz

A few smaller things.
The avanced API outlined in the PR would have been nice, but I assume that most people use uiRouter anyway at this point.

Narretz · 2018-05-25T09:49:53Z

src/ngRoute/route.js

+   *      If the option is set to `false` and the URL in the browser changes, then a `$routeUpdate`
+   *      event is broadcasted on the root scope (without reloading the route).
+   *
+   *      **Note:** This option has no effect if `reloadOnUrl` is set to false.


I think this should be more prominent, i.e. wrap it into a note block

Narretz · 2018-05-25T09:53:18Z

src/ngRoute/route.js

@@ -544,8 +556,9 @@ function $RouteProvider() {
     * @name $route#$routeUpdate
     * @eventType broadcast on root scope
     * @description
-     * The `reloadOnSearch` property has been set to false, and we are reusing the same
-     * instance of the Controller.
+     * Any of the `reloadOnSearch` and `reloadOnUrl` properties has been set to false and we are


I would change this to something like `This event is fired if we are reusing the same instance ... because any of ...

Narretz · 2018-05-25T12:30:48Z

src/ngRoute/route.js

-      preparedRouteIsUpdateOnly = preparedRoute && lastRoute && preparedRoute.$$route === lastRoute.$$route
-          && angular.equals(preparedRoute.pathParams, lastRoute.pathParams)
-          && !preparedRoute.reloadOnSearch && !forceReload;
+      preparedRouteIsUpdateOnly =


This would be even more readable if it was in a separate fnction

Functionized.

Narretz · 2018-05-25T12:36:04Z

test/ngRoute/routeSpec.js

      });
    });


+    describe('reload', function() {


Can this describe be a bit more explicit? There's already a top-level describe with this name. How about with reload()

I don't see a top-level describe with the same name 😕.

But I agree the descriptions are not great (I copied them from the existing reloadOnSearch tests 😳).
I've updated both to with `$route.reload()` (since they are testing how the configs work in conjuction with $route.reload()).

Yeah, I didn't see the reload() block was in reloadOnSearch

Narretz · 2018-05-25T12:37:29Z

test/ngRoute/routeSpec.js

-    it('should reload a route when reloadOnSearch is enabled and .search() changes', function() {
-      var reloaded = jasmine.createSpy('route reload');
+  describe('reloadOnUrl', function() {
+    it('should reload a route when `reloadOnUrl` is enabled and `.url()` changes', function() {


The wording is inconsistent in the changes - mostly it's enabled but further below it's true (I prefer the latter).

Made consistent.

gkalpak · 2018-05-26T06:08:01Z

Thx, @Narretz!
Comments adressed. PTAL

Narretz · 2018-05-26T14:15:25Z

I think you forgot to push the changes @gkalpak 👀

gkalpak · 2018-05-26T19:43:46Z

Ooops 😳 Definitely pushed now 😃

Narretz · 2018-06-06T08:17:05Z

LGTM - maybe @petebacondarwin should take another look?

petebacondarwin · 2018-06-06T08:27:44Z

test/ngRoute/routeSpec.js

      $httpBackend.when('GET', 'bar.html').respond('bar');
+      $httpBackend.when('GET', 'baz.html').respond('baz');


Does this change have any effect?

No afaict 😁
_{(Except for ordering definitions in proper foo-bar-baz order, which is as great an effect as a change can have 😇)}

petebacondarwin · 2018-06-06T08:32:05Z

test/ngRoute/routeSpec.js

+
+        inject(function($location, $rootScope, $routeParams) {
+          $rootScope.$on('$routeChangeStart', routeChange);
+          $rootScope.$on('$routeChangeSuccess', routeChange);


NIT: if we care to test that both these are being triggered then they should have different spies. Otherwise we don't know that only one of the events is being triggered twice rather than each being triggered only once.

We have other tests to verify that these are called correctly on route change.
Here we mainly want to verify that the routeChangeStart/Success combo is emitted and routeUpdate isn't.

petebacondarwin · 2018-06-06T08:35:55Z

test/ngRoute/routeSpec.js

+        expect(routeChangeSuccess).toHaveBeenCalledOnce();
+        expect($log.debug.logs).toEqual([['initialized']]);
+
+        $log.reset();


petebacondarwin · 2018-06-06T08:36:05Z

test/ngRoute/routeSpec.js

+          expect($log.debug.logs).toEqual([['initialized']]);
+          expect($routeParams).toEqual({param: 'bar'});
+
+          $log.reset();


Actually, these calls are needed, because there is an afterEach() call that asserts the log is empty:

angular.js/test/helpers/testabilityPatch.js

Line 50 in 84d80be

$log.assertEmpty();

Weird because I thought there are other tests that do not call $log.reset() at the end but do cause logging...

By calling reset we are preventing the afterEach from checking that other logs were not posted. In any case this is a bit counter-intuitive. Perhaps we should have a helper that checks the log and then removes that item?

petebacondarwin · 2018-06-06T08:39:38Z

test/ngRoute/routeSpec.js

+    );
+
+
+    it('should reload when `reloadOnSearch` is true and url differs only in route path param',


'should reload when reloadOnSearch is ~~true~~ false and ...' ?

petebacondarwin

Few minor nits but OLGTM

Enables users to specify that a particular route should not be reloaded after a URL change (including a change in `$location.path()`), if the new URL maps to the same route. The default behavior is still to always load the matched route when any part of the URL changes. Related to angular#1699, angular#5860, angular#14999 (potentially closing the first two). Fixes angular#7925

…option

gkalpak · 2018-06-07T14:16:16Z

Fixed up and rebased.

…option

Enables users to specify that a particular route should not be reloaded after a URL change (including a change in `$location.path()`), if the new URL maps to the same route. The default behavior is still to always load the matched route when any part of the URL changes. Related to #1699, #5860, #14999 (potentially closing the first two). Fixes #7925 Closes #15002

googlebot added the cla: yes label Aug 7, 2016

gkalpak added type: feature component: ngRoute frequency: moderate needs: review type: perf needs: public api severity: inconvenient labels Aug 7, 2016

gkalpak modified the milestones: 1.6.x, Backlog Aug 7, 2016

gkalpak mentioned this pull request Aug 8, 2016

Directly modifying the URL path & search without any side effects. #3789

Open

gkalpak force-pushed the feat-route-reload-on-path branch from 748cb40 to 4f54a92 Compare January 3, 2017 14:02

gkalpak mentioned this pull request Oct 2, 2017

feat(ngRoute): add method $route.silent(path) for change location path without triggering route controller #5860

Closed

petebacondarwin modified the milestones: Backlog, 1.7.0 Jan 29, 2018

petebacondarwin modified the milestones: 1.7.0, 1.6.10 Feb 26, 2018

petebacondarwin modified the milestones: 1.6.10, 1.7.x May 2, 2018

gkalpak self-assigned this May 9, 2018

petebacondarwin self-assigned this May 14, 2018

gkalpak force-pushed the feat-route-reload-on-path branch from 4f54a92 to 96c71b0 Compare May 23, 2018 19:13

Narretz suggested changes May 25, 2018

View reviewed changes

gkalpak force-pushed the feat-route-reload-on-path branch from 96c71b0 to daec562 Compare May 26, 2018 19:43

Narretz approved these changes Jun 6, 2018

View reviewed changes

petebacondarwin reviewed Jun 6, 2018

View reviewed changes

petebacondarwin approved these changes Jun 6, 2018

View reviewed changes

gkalpak added 2 commits June 7, 2018 17:10

fixup! feat($route): add support for the reloadOnUrl configuration …

c767d3e

…option

gkalpak force-pushed the feat-route-reload-on-path branch from daec562 to 956c727 Compare June 7, 2018 14:15

fixup! feat($route): add support for the reloadOnUrl configuration …

a9b6b21

…option

gkalpak force-pushed the feat-route-reload-on-path branch from 956c727 to a9b6b21 Compare June 7, 2018 20:47

gkalpak closed this in 60069e6 Jun 8, 2018

This was referenced Jun 8, 2018

feat($location/$route): support changing location without side effects/route reloading #14999

Closed

Option on $location to allow hash/path change w/o reloading the route #1699

Closed

gkalpak deleted the feat-route-reload-on-path branch June 8, 2018 13:17

dependencies bot mentioned this pull request Jun 8, 2018

Update 6 dependencies from npm JetBrains/ring-ui#281

Closed

Shazwazza mentioned this pull request Jun 12, 2019

v8 - overlays after creating new content (redirect) reference a stale controller umbraco/Umbraco-CMS#4988

Closed

		$httpBackend.when('GET', 'bar.html').respond('bar');
		$httpBackend.when('GET', 'baz.html').respond('baz');

		);


		it('should reload when `reloadOnSearch` is true and url differs only in route path param',

feat($route): add support for the reloadOnUrl configuration option #15002

feat($route): add support for the reloadOnUrl configuration option #15002

Conversation

gkalpak commented Aug 7, 2016 • edited Loading

gkalpak commented Aug 7, 2016 • edited Loading

gkalpak commented Aug 7, 2016

Mischi commented Aug 16, 2016

gkalpak commented Aug 17, 2016

Narretz commented Nov 29, 2016 • edited Loading

gkalpak commented Nov 29, 2016

miwillhite commented Nov 3, 2017

Narretz commented Nov 4, 2017

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak May 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented May 26, 2018

Narretz commented May 26, 2018

gkalpak commented May 26, 2018

Narretz commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

gkalpak commented Jun 7, 2018

feat($route): add support for the `reloadOnUrl` configuration option #15002

feat($route): add support for the `reloadOnUrl` configuration option #15002

gkalpak commented Aug 7, 2016 •

edited

Loading

gkalpak commented Aug 7, 2016 •

edited

Loading

Narretz commented Nov 29, 2016 •

edited

Loading

gkalpak May 26, 2018 •

edited

Loading