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.

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

Problem with reloadOnSearch: false - change in stateParams doesn't reload #1079

Closed
OriaLogic opened this issue May 15, 2014 · 35 comments
Closed
Assignees
Labels
Milestone

Comments

@OriaLogic
Copy link

Here is my issue:

I navigate through a list of links, that display different images depending on the date in the search parameters (?from&to).

Those search parameters should not trigger a state reload. Finally, here is my state declaration:

state('links/:linkId', {
  url: "/links/linkId?from&to",
  reloadOnSearch: false,
  templateUrl: ...
  Controller: ...
});

But then a huge problem shows up! When I try to navigate through my different links, reload doesn't happen anymore, so that I cannot pass from one link to another.

After some digging in the code, I found out that when reloadOnSearch equals to false, triggers are always blocked because there's absolutely no checking of whether the changing parameters are search parameters or path parameters.

So actually, reloadOnSearch doesn't work as intended and should be renamed reloadOnStateParamsChange.

I fixed the issue by checking if the state parameters that changed are search parameters or not, which seems to work fine.

Please tell me if I'm mistaking somewhere or if it is relevant, and also if you would be interested in a pull request or a plunkr.

Thanks in advance!

@timkindberg
Copy link
Contributor

You are right that it doesn't care about which type of param. It was a quick fix for a large problem. A more official feature is now in master; typed params. We may even ditch this config property now.

@nateabele you think we should get rid of reloadOnSearch?

@nateabele
Copy link
Contributor

@timkindberg Yeah, it's getting killed off in the next release.

@TEHEK
Copy link

TEHEK commented Jun 25, 2014

Hi!

Please consider those of us who migrate from ngRouter.

I've recently switched my large application from using ngRouter to uiRouter. And it wasn't easy...

The main difference here is that reloadOnSearch in ngRouter means that the same instance of the controller is going to be re-used. Typed query params are nice, but they are not the same. Basically, whenever a param changes, a new instance of controller is instantiated. Re-factoring this can be pretty huge.

Any way we can fix the reloadOnSearch instead?

homerjam added a commit to homerjam/ui-router that referenced this issue Aug 11, 2014
state will now be reloaded if `state.reloadOnSearch = false` when doing `state.go('stateName', {reload: true})`
@pholly
Copy link

pholly commented Aug 21, 2014

@timkindberg @nateabele I also don't see how typed parameters can replace reloadOnSearch's documented purpose. I want to define a query string parameter on a state and not reload the state if the query string parameter changes while I'm on that state.

@pholly
Copy link

pholly commented Aug 21, 2014

@TEHEK A workaround is to make your state abstract and define reloadOnSearch true (which it is by default) and create a child state that only defines a url with the query string parameters you want to not cause a state reload.

E.g.:

.state('contacts', {
  url: '/contacts',
  abstract: true,
  template: '<h1>Contacts</h1>',
  controller: ['$scope', 
      function($scope) {
         //will not reload controller or view when contacts.content state changes
      }], 
   onEnter: function() {
      console.log("OnEnter: contacts");
  }
})
.state('contacts.content', {
  url: '?param1&param2&interestedDirectivesWillWatchMe',
  onEnter: function() {
      console.log("OnEnter: contacts.content");
  }
})

@ghost
Copy link

ghost commented Sep 20, 2014

I'm having the same problem. I've divided up my stateParams into a structure where all the optional params that I don't want to cause a reload are in the querystring, and all the required params are in the base URL. But I can't navigate to the same state with a different param (no matter what I try, the state will not reload because I have reloadOnSearch set to false) using $state.go or transtitionTo... I've tried all the options {reload: true, inherit: false, location: true}. I've also tried to call $state.reload() which also did absolutely nothing.

I definitely need something like reloadOnSearch to allow some stateParams to change without the state reloading... but I need other params to cause a reload. If reloadOnSearch did what it's name implies (only prevent reload when the querystring params change), it would be fine... but it appears to kill all reloading, even forced ones.

@nateabele
Copy link
Contributor

@pholly @TEHEK In and of themselves, typed parameters don't replace reloadOnSearch. What will replace reloadOnSearch is dynamic parameters, which is currently partially implemented in master, and will be fully-realized in the next release, this will give you per-parameter (query string parameters or normal URL parameters) disabling of controller reload, and the ability to watch parameter changes.

@cmcnamara87
Copy link

+1. Tried the workaround from @pholly, but because the optional parameters are on a child state, the parent controller doesn't have access to them through $stateParams. $state.params also doesn't work as it is first loaded with the parent's params, and then the child's params are added, but since the parameters are optional, there is no way to know when/if that will happen in the parent.

@pholly
Copy link

pholly commented Oct 21, 2014

@cmcnamara87 You can use $scope.$watch to watch $state.params in the parent controller or in any directives in the view(s) of the parent state or any ancestor state.

I typically need to watch a subset of $state.params so I use $scope.$watchCollection with an anonymous object as a parameter.

E.g.

//this assumes $state is on the $scope - $scope.$state
$scope.$watchCollection("{param1: $state.params.param1, param3: $state.params.param3}", function(newVal, oldVal, scope){
  //$state.params.param1 or $state.params.param3 changed - do stuff
  //old values are in oldVal.param1 and oldVal.param3
});

@christopherthielen christopherthielen self-assigned this Oct 21, 2014
@christopherthielen christopherthielen added this to the 1.0.0-preview milestone Nov 21, 2014
@just-boris
Copy link
Contributor

original angular ngRoute module has same reloadOnSearch config. But they ignore reloads only search params not url segments. And it's good for me, so I propose to do same behavior there and add another option reloadOnParamsChange if needed

@nawlbergs
Copy link

I can also confirm this issue. I'm using search parameters to store minor ui variables within a route/view.
e.g.
#/user/123/reports?from=today&to=tomorrow
and cannot change the users id.

@UmairB
Copy link

UmairB commented Feb 19, 2015

My hilarious hack for this is as follows

//set the reloadOnSearch on state to false initially in some code block
$state.current.reloadOnSearch = false;

// update query string here
$location.search({"from": 1, "to": 2});

//revert reloadOnSearch on the state!
$timeout(function () {
$state.current.reloadOnSearch = undefined;
});

@jpmckearin
Copy link

@nateabele @christopherthielen I noticed dynamic parameters has yet to be merged into master. Is there any reason that I can't be of assistance and just do the merge and submit a PR?

@christopherthielen
Copy link
Contributor

@jpmckearin yes, unfortunately we're gated on the 1.0 refactor before we add any additional major features

@straker
Copy link

straker commented Mar 25, 2015

So I know there is a planned "fix" with dynamic parameters, but if you're gated for an undetermined amount of time from merging it in, could we at least get the fix of having reloadOnSearch actually reload on path changes now?

@mjrk
Copy link

mjrk commented Apr 16, 2015

+1

@eddiemonge
Copy link
Contributor

yeah a patch fix would be nice. i know that makes refactors hard but it is a long standing bug

@skeisman
Copy link

skeisman commented May 2, 2015

This is a very important feature for my project.

If I make several changes to search params and then navigate to a child state, when I call $state.go('^') only the first search params are remembered.

I have spend several hours reading through the source code to try to fix this myself but it seems like it would take me a while to get up to speed.

BTW thanks for creating such a useful router.

@skeisman
Copy link

skeisman commented May 8, 2015

When I click an "a" tag that only changes the search params on a page that has reloadOnSearch:false nothing happens. This seems like a related bug.

For example: state1 uses reloadOnSearch:false.
While on /state1?foo=bar I click a normal href link to /state1?foo=bar&baz=ban nothing happens. The url does not change.

@eddiemonge
Copy link
Contributor

Why do you have a normal href link and not a ui-sref link?

@skeisman
Copy link

skeisman commented May 9, 2015

The bug I have described above also fails with ui-sref. I wanted to describe the problem with as few moving parts as possible. UI router ignores href clicks that do not change the state when you have reloadOnSearch:false

@eddiemonge
Copy link
Contributor

Right, thats what reloadOnSearch: false means, don't reload the state when a query param changes.

@skeisman
Copy link

When I click an href link the url does not change when reloadOnSearch:false. That means reloadOnSearch:false is breaking normal browser functionality.

My specific problem aside, A better way to make reloadOnSearch:false work is that the state should change but the view would not reload. This will cause $stateParams to be correct. This will cause $state.go('^') to work correctly and a whole bunch of other edge cases to work correctly. In its current state reloadOnSearch:false is nearly unusable because it breaks so many other things.

@ceoaliongroo
Copy link

I my case, i works for me create a decorator to keep sync $urlRouter and $location

    /**
       * Update update params values with reloadOnSearch:false.
       *
       * @param params
       *  Object of the change for the $stateParams.
       */
      $delegate.refreshUrlWith = function refreshUrlWith(params) {
        $location.search(extend(copy($stateParams),
          $location.search(),
          params
        ));
        $urlRouter.update(true);
      };

metamatt pushed a commit to metamatt/ui-router that referenced this issue May 18, 2015
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 angular-ui#1079. Helps with one of the cases broken in angular-ui#582.
@metamatt
Copy link

I just sent a PR for this issue which I believe is a correct fix for the 0.2 codebase as it stands. The dynamic parameter stuff, from what I've been able to glean from in-passing references in this and a few other issue reports and PRs, looks pretty cool and I look forward to being able to use that, but I hope we can take this fix for the interim period before that lands.

@christopherthielen
Copy link
Contributor

@metamatt Thanks for the PR. I modified your commit and merged it in manually.

@christopherthielen
Copy link
Contributor

This plunkr demonstrates 0.2.15 reloadOnSearch functioning: http://plnkr.co/edit/5OPoKUDnn98JiKFlyEYL?p=preview

@ghost
Copy link

ghost commented May 19, 2015

Works for me, awesome job @metamatt!

@drothlis
Copy link

What will replace reloadOnSearch is dynamic parameters, which is currently partially implemented in master, and will be fully-realized in the next release, this will give you per-parameter (query string parameters or normal URL parameters) disabling of controller reload, and the ability to watch parameter changes.

@nateabele where is this documented? I don't see any mention of it at https://angular-ui.github.io/ui-router/site/#/api/ui.router.util.type:Type -- any tips/pointers would be much appreciated! :-)

@nateabele
Copy link
Contributor

@drothlis Not super well-documented, sadly: https://christopherthielen.github.io/ui-router/1.0-alpha01/classes/params.param.html#dynamic

You define one like this:

$stateProvider.state("foo", {
  url: "/:bar",
  params: {
    bar: { dynamic: true }
  }
});

@christopherthielen
Copy link
Contributor

this is the temporary home for 1.0 alpha docs: http://angular-ui.github.io/ui-router/feature-1.0

http://angular-ui.github.io/ui-router/feature-1.0/interfaces/params.paramdeclaration.html

dynamic is still not documented, sorry

@drothlis
Copy link

@nateabele, @christopherthielen: Thanks, that's very helpful! :-)

@ghost
Copy link

ghost commented Sep 5, 2016

Is this in 1.5.9?
Reloading the template is not acceptable to me because I've got a (hopefully) persistent STOMP controller that is in constant communication. When I switch from /item/2 to /item/3 I'd like the user to be able to bookmark that. Instead the STOMP controller (which is its own controller, so doesn't get reloaded) goes through all the steps of initiating communications again because the template gets reloaded.

My app.js includes this:
.state('thing', {
url: '/thing/:id',
templateUrl: '/tpl/thing.html',
params: { id: { dynamic: true }}
});
Did I get something wrong? I'm running 1.5.8 at the moment.

Thanks.

@christopherthielen
Copy link
Contributor

That looks fine. It shouldn't reload when the parameter changes. Are you using ui-router 1.0 beta? Dynamic params are not in the legacy versions.

@ghost
Copy link

ghost commented Sep 5, 2016

@christopherthielen Thank you. I use Bower for most of my packages, and it seems like for some reason they only have the legacy version on Bower. NPM'd ui-router 1.0 beta works like a charm!

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

Successfully merging a pull request may close this issue.