Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix($state): reloadOnSearch should not affect non-search param changes. #1962

Closed
wants to merge 1 commit into from
Closed

fix($state): reloadOnSearch should not affect non-search param changes. #1962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 40 additions & 6 deletions src/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}.
Expand Down Expand Up @@ -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)) {
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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')
Expand Down
18 changes: 17 additions & 1 deletion test/stateSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};
Expand All @@ -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" })
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -940,6 +955,7 @@ describe('state', function () {
'OPT',
'OPT.OPT2',
'RS',
'RSP',
'about',
'about.person',
'about.person.item',
Expand Down