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

Query Params replace doesn't work if refreshModel is also used #10577

Closed
Panman82 opened this issue Mar 5, 2015 · 14 comments · Fixed by tildeio/router.js#198
Closed

Query Params replace doesn't work if refreshModel is also used #10577

Panman82 opened this issue Mar 5, 2015 · 14 comments · Fixed by tildeio/router.js#198

Comments

@Panman82
Copy link
Contributor

Panman82 commented Mar 5, 2015

Currently working with Ember 1.11.0-beta.4 and noticed that the query param replace option doesn't work with refreshModel. If the refreshModel option is true then a new history entry will be added. I assume this is due to the fact that the route model hook is refreshed. Is this expected or a bug?

Here is a jsbin showing the issue: http://emberjs.jsbin.com/tevemudera/1/edit?html,js,output
But if refreshModel is commented out it works: http://emberjs.jsbin.com/tevemudera/2/edit?html,js,output

@floriankrb
Copy link

I have the same issue. I would like to have {refreshModel:true, replace:true}, in my route's queryParams.
From what I see, here https://github.com/emberjs/ember.js/blob/v1.10.0/packages/ember-routing/lib/system/route.js#L687 the code using 'replace' is fine.
But when using {refreshModel:true}, it triggers the refresh() method here :https://github.com/emberjs/ember.js/blob/v1.10.0/packages/ember-routing/lib/system/route.js#L955
this "return this.router.router.refresh(this); " is unclear to me. But I expect that it doesn't propagate the value of 'replace' and causes the issue.

@alexspeller
Copy link
Contributor

@machty are you aware of this one? Looks like it hasn't been labelled yet

@alexspeller
Copy link
Contributor

Reproduction: https://jsfiddle.net/w3sj0mkt/19/

Back button should not change input value after you have typed in it

@stevesims
Copy link

+1
Also seeing this issue - would be very useful if it could be fixed, failing that a work-around would be cool

@workmanw
Copy link

workmanw commented Jun 8, 2015

I wonder if #10945 is ultimately related to this issue as well.

@Fryie
Copy link

Fryie commented Jun 29, 2015

Seems to be related to this line:

if (transition.queryParamsOnly && replaceUrl !== false) {

transition.queryParamsOnly appears to be set to false when refreshing the model, so the replaceUrl variable is not read from the declaration.

edit: updated link with reference to original code.

@moha297
Copy link

moha297 commented Jul 6, 2015

I am seeing a similar issue when using 'as' and 'replace' together in a queryParam definition.

@snewcomer
Copy link
Contributor

Does this issue have any potential solutions?

@Fryie
Copy link

Fryie commented Dec 14, 2015

Well, the code in question changed quite a bit since I reported that line, so it's possible that this has been fixed in the meantime.

@ramybenaroya
Copy link

I am with 2.3.0-beta.1 and it still happens

@Fryie
Copy link

Fryie commented Dec 14, 2015

Ah, yes, the line just moved a little. Still the same problem, I guess:

if (transition.queryParamsOnly && replaceUrl !== false) {

@ehntoo
Copy link

ehntoo commented Feb 11, 2016

I've also encountered something I think is related to this bug, at least if @Fryie's hunch is correct.

When queryParams are declared as reloadModel: true, an observer is watching one of your query param values to change another, and you use the guide-recommended sticky query param resetting methodology ( https://guides.emberjs.com/v2.3.0/routing/query-params/#toc_sticky-query-param-values ), the param that's being set by an observer gets squashed back to defaults. Details below.

I was implementing a sortable/filterable paged list view with server-side pagination, and where the sort/filter/page state was represented as queryParams. I planned on using refreshModel: true for the query params so I could really easily leverage ember loading substate magic on page/filter/sort changes.

Applying the sticky query params methodology from the guides to clear filters/whatnot for subsequent controller activations worked fine, right up until I realized it would be nice to punt the user back to the first page when the filter criteria changed.

I implemented the functionality to push the user back to the first page using an observer on the filter criteria, but it got triggered every time the page was changed as well. It turns out the resetController hook was being called with isExiting as true on every query param change due to my use of refreshModel: true. :(

@ehntoo
Copy link

ehntoo commented Feb 11, 2016

Worked around the issue by intercepting the queryParamsDidChange action in my route and manipulating query params such that when a param is changed that should reset the current page, the pageNumber query param is removed from the totalPresent arg and added to the removed arg.

Far from ideal, as this isn't documented to be public API. :/

  actions: {
    queryParamsDidChange(changed, totalPresent, removed) {
      const propertiesRequiringPageReset = ['sortField', 'sortAscending', 'filterName', 'filterValue'];
      const propertyDidChange = function(propName) {
        return (changed.hasOwnProperty(propName) || removed.hasOwnProperty(propName));
      };

      // If the page number has changed, we don't want to clobber that
      if ((!propertyDidChange('pageNumber')) &&
          (propertiesRequiringPageReset.any(propertyDidChange))) {
        // One of the properties that will bump the user back to the first page
        // has changed. Intercept and modify the args to make the router think
        // the pageNumber already changed with everything else. *grimace*
        const previousPageNum = totalPresent.pageNumber;
        delete changed.pageNumber;
        delete totalPresent.pageNumber;
        removed.pageNumber = previousPageNum;
      }
      // Carry on, nothing to see here.
      return this._super(...arguments);
    }
  }

@mike-north
Copy link
Contributor

This bug is still reproducible as of 2.9.0-beta.5

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

Successfully merging a pull request may close this issue.