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

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

Closed
wants to merge 1 commit into from

Conversation

metamatt
Copy link

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.

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
Author

Review notes: What's here works for my purposes and passes the test suite including a new test for the #1079 case it fixes. It may not be the optimal way of spelling out these concepts for your codebase and I'm happy to adjust this with feedback and get it right.

@christopherthielen christopherthielen added this to the 0.2.14 milestone May 19, 2015
@christopherthielen christopherthielen self-assigned this May 19, 2015
@christopherthielen
Copy link
Contributor

I modified and manually merged this in commit 6ca0d77

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 this pull request may close these issues.

Problem with reloadOnSearch: false - change in stateParams doesn't reload
2 participants