From 49ec7264fbf883886f323da094866c3d8b222346 Mon Sep 17 00:00:00 2001 From: Matt Ginzton Date: Mon, 18 May 2015 14:57:51 -0700 Subject: [PATCH] fix($state): reloadOnSearch should not affect non-search param changes. The handling of `reloadOnSearch: false` caused ui-router to avoid reloading the state in more cases than it should. The designed and documented behavior of this flag is to avoid state reload when the URL search string changes. It was also avoiding state reload when the URL path (or any non-search parameters to the state) changed, and even when state reload was explicitly requested. This change - flips the name of shouldTriggerReload (and the accompanying guard boolean, skipTriggerReloadCheck) to match the direction of the logic: shouldSkipReload and allowSkipReloadCheck - teaches shouldSkipReload to look at the types of the differing parameters, and only skip the reload if the only parameters that differ were search parameters - pulls the test for options.reload to the front of the complex boolean expression. (I think calling $state.reload() explicitly should reload a state even if it has reloadOnSearch:false. But I don't understand exactly why the test for this was where it was, and maybe there's a good reason I'm missing. Also, if the behavior I favor is deemed correct, we could also achieve that by setting allowSkipReloadCheck=false for all truthy values of options.reload, namely, if options.reload===true, and then shouldSkipReload wouldn't even need to look at options. I left a comment about this in the source too and would appreciate feedback.) Fixes #1079. Helps with one of the cases broken in #582. --- src/state.js | 46 ++++++++++++++++++++++++++++++++++++++++------ test/stateSpec.js | 18 +++++++++++++++++- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/state.js b/src/state.js index 62d504ba0..d33f71d09 100644 --- a/src/state.js +++ b/src/state.js @@ -954,7 +954,7 @@ function $StateProvider( $urlRouterProvider, $urlMatcherFactory) { * have not changed, aka a reload of the same state. It differs from reloadOnSearch because you'd * use this when you want to force a reload when *everything* is the same, including search params. * if String, then will reload the state with the name given in reload, and any children. - * if Object, then a stateObj is expected, will reload the state found in stateObj, and any chhildren. + * if Object, then a stateObj is expected, will reload the state found in stateObj, and any children. * * @returns {promise} A promise representing the state of the new transition. See * {@link ui.router.state.$state#methods_go $state.go}. @@ -1002,7 +1002,7 @@ function $StateProvider( $urlRouterProvider, $urlMatcherFactory) { // Starting from the root of the path, keep all levels that haven't changed var keep = 0, state = toPath[keep], locals = root.locals, toLocals = []; - var skipTriggerReloadCheck = false; + var allowSkipReloadCheck = true; if (!options.reload) { while (state && state === fromPath[keep] && state.ownParams.$$equals(toParams, fromParams)) { @@ -1020,7 +1020,9 @@ function $StateProvider( $urlRouterProvider, $urlMatcherFactory) { throw new Error("No such reload state '" + (isString(options.reload) ? options.reload : options.reload.name) + "'"); } - skipTriggerReloadCheck = true; + // Question for PR review: Why do we not reset this for options.reload === true?! + // (shouldSkipReload had an embedded check for options.reload but it wouldn't need it if we did this) + allowSkipReloadCheck = false; while (state && state === fromPath[keep] && state !== reloadState) { locals = toLocals[keep] = state.locals; @@ -1034,7 +1036,7 @@ function $StateProvider( $urlRouterProvider, $urlMatcherFactory) { // TODO: We may not want to bump 'transition' if we're called from a location change // that we've initiated ourselves, because we might accidentally abort a legitimate // transition initiated from code? - if (!skipTriggerReloadCheck && shouldTriggerReload(to, from, locals, options)) { + if (allowSkipReloadCheck && shouldSkipReload(to, toParams, from, fromParams, locals, options)) { if (to.self.reloadOnSearch !== false) $urlRouter.update(); $state.transition = null; return $q.when($state.current); @@ -1429,11 +1431,43 @@ function $StateProvider( $urlRouterProvider, $urlMatcherFactory) { return $state; } - function shouldTriggerReload(to, from, locals, options) { - if (to === from && ((locals === from.locals && !options.reload) || (to.self.reloadOnSearch === false))) { + function shouldSkipReload(to, toParams, from, fromParams, locals, options) { + // If reload was not explicitly requested + // and we're transitioning to the same state we're already in + // and the locals didn't change + // or they changed in a way that doesn't merit reloading + // (reloadOnParams:false, or reloadOnSearch.false and only search params changed) + // Then skip the reload. + // NB: If we skip the reload, $stateParams does not get updated. So people have to manage + // $location.search() entirely manually, not via $stateParams, if using reloadOnSearch: false. + if (!options.reload && to === from && (locals === from.locals || (to.self.reloadOnSearch === false && onlySearchParamsChanged(from, fromParams, to, toParams)))) { return true; } } + + function noteNonSearchParams(params, nonSearchParamsHash) { + for (var key in params) { + if (key.indexOf('$$') !== 0 && params[key].location !== 'search') { + nonSearchParamsHash[key] = true; + } + } + } + + function onlySearchParamsChanged(from, fromParams, to, toParams) { + // Identify whether all the parameters that differ between `fromParams` and `toParams` were search params. + // Return true if the only differences were in search params, false if there were differences in other params. + // Algorithm: build a set of keys of params that are not search params. + // Walk that set, comparing the actual values between the fromParams and toParams. + var nonSearchParams = {}; + noteNonSearchParams(from.params, nonSearchParams); + noteNonSearchParams(to.params, nonSearchParams); + for (var key in nonSearchParams) { + if (fromParams[key] !== toParams[key]) { + return false; + } + } + return true; + } } angular.module('ui.router.state') diff --git a/test/stateSpec.js b/test/stateSpec.js index f2aa4dc46..7b3a4f36d 100644 --- a/test/stateSpec.js +++ b/test/stateSpec.js @@ -29,6 +29,7 @@ describe('state', function () { HH = { parent: H }, HHH = {parent: HH, data: {propA: 'overriddenA', propC: 'propC'} }, RS = { url: '^/search?term', reloadOnSearch: false }, + RSP = { url: '^/:doReload/search?term', reloadOnSearch: false }, OPT = { url: '/opt/:param', params: { param: "100" } }, OPT2 = { url: '/opt2/:param2/:param3', params: { param3: "300", param4: "400" } }, AppInjectable = {}; @@ -55,6 +56,7 @@ describe('state', function () { .state('OPT', OPT) .state('OPT.OPT2', OPT2) .state('RS', RS) + .state('RSP', RSP) .state('home', { url: "/" }) .state('home.item', { url: "front/:id" }) @@ -212,7 +214,20 @@ describe('state', function () { }); $q.flush(); expect($location.search()).toEqual({term: 'hello'}); - expect(called).toBeFalsy(); + expect(called).toBeFalsy(); + })); + + it('does trigger state change for path params even if reloadOnSearch is false', inject(function ($state, $q, $location, $rootScope){ + initStateTo(RSP, { doReload: 'foo' }); + expect($state.params.doReload).toEqual('foo'); + var called; + $rootScope.$on('$stateChangeStart', function (ev, to, toParams, from, fromParams) { + called = true + }); + $state.transitionTo(RSP, { doReload: 'bar' }); + $q.flush(); + expect($state.params.doReload).toEqual('bar'); + expect(called).toBeTruthy(); })); it('ignores non-applicable state parameters', inject(function ($state, $q) { @@ -940,6 +955,7 @@ describe('state', function () { 'OPT', 'OPT.OPT2', 'RS', + 'RSP', 'about', 'about.person', 'about.person.item',