emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.45k stars 4.21k forks source link

setting a controller property in `setupController` does not update the query param #5465

Closed tikotzky closed 5 years ago

tikotzky commented 10 years ago

if you have a Controller defined as

App.IndexController = Ember.Controller.extend({
  queryParams: ['foo'],
  foo: ''
});

and in the Routes setupController you do something like...

setupController: function(controller, model){
  controller.set('model', model);
  controller.set('foo', 'bar'); // <-- setting this should update the query param
}

The query param does not get set in the url.

You can see a working bin here: http://emberjs.jsbin.com/dekuj/1/edit

rwjblue commented 10 years ago

Fixed with a Ember.run.later (to ensure that things have fully materialized first): http://emberjs.jsbin.com/kowoqo/1/edit

@machty - What is the expected behavior here?

machty commented 10 years ago

I thought we had explicit tests for this... checking.

machty commented 10 years ago

I think this is a bug... we have tests for this but only in the context of a transitionTo and not the initial app bootup.

working: http://jsbin.com/mejeto/1#/about

rahult commented 10 years ago

I have added a failing test for this bug https://github.com/emberjs/ember.js/pull/5473 Will try to take a stab at fixing this

igor10k commented 9 years ago

I think I stumbled upon related issue, so I'm not sure if it needs separate ticket here in GitHub.

export default Ember.Controller.extend({
    queryParams: ['modal'],
    modal: null,
    handleModal: function () {
        this.send('openModal', this.get('modal'));
    }.observes('modal')
});
export default Ember.Route.extend({
    actions: {
        openModal: function (name) {
            return this.render(name, {
                into: 'application',
                outlet: 'modal'
            });
        }
    }
});

This works in all cases besides the bootup when it throws Error while processing route: index Nothing handled the action 'openModal'. If you did handle the action, this error can be caused by returning true from an action handler in a controller, causing the action to bubble.

Can be fixed using Ember.run.next

handleModal: function () {
    Ember.run.next(this, function () {
        this.send('openModal', this.get('modal'));
    });
}.observes('modal')

though it feels wrong.

JSBins: Without Ember.run.next throws error With Ember.run.next prints 'redered' in console

ctruelson commented 9 years ago

@Igor10k I'm running into this exact same issue, modal and all. Wrapping it in Ember.run.next fixes it, but it feels wrong to me as well.

wagenet commented 9 years ago

Can anyone confirm that this is still an issue in 1.8?

tikotzky commented 9 years ago

@wagenet yes this is still an issue in 1.8: http://emberjs.jsbin.com/lezuhu/1/edit?html,js,output

and canary: http://emberjs.jsbin.com/doyohe/1/edit?html,js,output

rwjblue commented 9 years ago

@machty - Thoughts?

eccegordo commented 9 years ago

I think this is also a bug for me too using 1.8.1. What I want to do is call a special method on my controller every time a specific queryParam changes. I have a router that looks something like this. Any advice?

// ...snip...

// for pagination
queryParams: {
  offset: {
    refreshModel: true
  }
},

model: function(params) {
  // returns model
},

setupController: function(controller, model) {
  this._super(controller, model);
  controller.set('model', model);

  // Fetch results is a method that does a bunch work
  // sets up some filters, generates a promise, resolves that promise
  // and then sets some array data on the controller once promise resolves.
  // offset is a pagination feature which determines which specific results
  // are fetched. I would like to be able to call fetchResults everytime the
  // offset queryParam is changed. 
  Ember.run.later(function() {
    controller.sendAction('fetchResults');
  })
}

// ...snip...
rwjblue commented 9 years ago

@machty - Ideas on where to start digging on this?

AlexanderPease commented 9 years ago

I'm having the same problem. this.set() in the controller will not update query parameter in the URL

AlexanderPease commented 9 years ago

App.SearchController = Ember.Controller.extend({ queryParams: ['domain'], domain: null,

domainField: Ember.computed.oneWay('domain'), actions: { searchSubmit: function() { this.set('domain', this.get('domainField')); // This sets the 'domain' parameter but doesn't modify the URL. So the model in App.SearchRoute is never being called as expected } }, });

App.SearchRoute = Ember.Route.extend({ queryParams: { domain: { refreshModel: true // Opt into full transition },

}, model: function(params) { var domain = params.queryParams.domain;

  return $.getJSON('/search?domain=usv.com&name=').then(function(resp){
  console.log(resp);
  return resp.data;
});

}, actions: { refresh: function() { Ember.Logger.log('Route is now refreshing...'); this.refresh(); } }, });

AlexanderPease commented 9 years ago

Can you please provide a work around in the interim?

AlexanderPease commented 9 years ago

To clarify: this.set() in my code will make the change to the Class object, but doesn't change the URL at all

backspace commented 9 years ago

This is ancient, but seems like it’s still a problem? I can’t figure out the circumstances, but periodically when I try to set a query parameter from the controller, it doesn’t propagate to the URL. I can check the value of the property on the controller and it’s correctly set, but the URL hasn’t changed. I tried using an Ember.run.next to no avail. Woe!

krzkrzkrz commented 9 years ago

Experiencing this issue too

greyhwndz commented 9 years ago

@rwjblue: Is there any update on this?

alex88 commented 8 years ago

Any update? Here even when changing the value from the controller, the url updates for a couple ms and it goes back to the old value, even if the controller has the new value in the variable

barelyknown commented 8 years ago

Here's a workaround:

In the controller, add an observer that changes the query param in the next run loop after it has been set. In this example, I wanted to remove the token query param from the URL and this approach works nicely.

import Ember from 'ember';

export default Ember.Controller.extend({
  queryParams: ['token'],

  token: null,

  unsetToken: Ember.observer('token', function() {
    if (this.get('token')) {
      Ember.run.next(this, function() {
        this.set('token', null);
      });
    }
  }),
});
raido commented 7 years ago

Just ran into this today, when I tried to set a query parameter from the setupController on boot. run.next seems like a workaround, but i think proper solution would be to have 2 routes and on boot first route would do transitionTo second route with the query parameter value.

Ember 2.8

Serabe commented 7 years ago

Marking issue a inactive as per our triage policy.

atremblay-seniorlink commented 7 years ago

I too am seeing this issue on Ember 2.11, trying to clear a query param in the controller has no effect, @barelyknown 's workaround gets me most of the way there but I would still consider this an open bug.

mwpastore commented 7 years ago

EDIT: Ignore all this jazz. There's some bad/wrong information here, and @locks pointed me towards a much better solution—with a query parameter instead of a dynamic segment—in-line immediately below. It binds the query parameter to a normal property with a shadow computed property that sanitizes and sets on get (my previous attempts involved binding the query parameter to a computed property that sanitized and set on set). Sorry for the noise!

The better solution... ```javascript // app/router.js Router.map(function() { this.route('shop'); }); // app/routes/shop.js export default Ember.Route.extend({ queryParams: { _selectedItemIndex: { replace: true } } }); // app/controllers/shop.js import computed from 'ember-macro-helpers/computed'; export default Ember.Controller.extend({ cart: Ember.service.inject(), queryParams: { _selectedItemIndex: 'i' }, _selectedItemIndex: 0, selectedItemIndex: computed('_selectedItemIndex', { get(index) { const itemsLength = this.get('cart.items.length'); if (isNaN(index) || index < 0 || index >= itemsLength) { index = 0; } return this.set('_selectedItemIndex', index); }, set(index) { return this.set('_selectedItemIndex', index); } }) }); ```
The original problem and solution... I lost the better part of yesterday to this issue that still persists in 2.14-beta. My use case is sanitizing a query parameter to use as an index into an Ember.Array. If it's malformed (e.g. NaN or a string) and/or out of bounds, it should use a default index, and update the URL to reflect the adjustment (so that events in response to future clicks trigger appropriately—I can explain this further to help make the case that this is not merely a cosmetic issue). I can't set the property from `setupController()` as it does not update the URL (as described in this issue). I can't set it in the next tick using Ember.run because that will take effect too late (i.e. an invalid index would have already been used to access the array). I can't set it once and then set it again in the next tick to update the URL because there is no property change (or at least that's what I suspect is the reason why it doesn't work). I can't use a computed property with a sanitizing setter in the controller because you can't bind a query parameter to a computed property. I can't use an observer on a normal property because you can't set the property you're observing. I can't use an observer with Ember.run due to the issue described above. I can't call `this.transitionTo({ queryParams: .. })` from `setupController()` or the `beforeModel()` hook or `controller.transitionToRoute({ queryParams: .. })` from `setupController()` due to #14606 and related issues. I can't set `refreshModel` to `true` for the query parameter because other stuff is happening in the `model()` hook that should not be repeated if this query parameter changes. Clearly there is a nexus of pain here. This issue, issue #14606, and the limitation of not being able to bind a query parameter to a computed property all combine to create a neat, frustrating little trap. Here's my workaround: I created a nested "select" route that simply takes the query parameter (or—as in the code below—a dynamic segment), sanitizes it, and sets the property on its parent controller. If the sanitized index is out of bounds it transitions to the nested index route, which in turn transitions back to the nested select route with the default index. The nested routes have no templates or controllers and the parent route has no outlet; they exist solely to handle this routing issue. ```javascript // app/router.js Router.map(function() { this.route('shop', function() { this.route('index', { path: '/' }); this.route('select', { path: '/:index' }); }); }); // app/routes/shop.js export default Ember.Route.extend({ setupController(controller, model) { this._super(...arguments); // some logic to set up the cart service }, model() { // fetch products to shop } }); // app/controllers/shop.js export default Ember.Controller.extend({ cart: Ember.service.inject(), selectedItemIndex: 0, actions: { selectItem(index) { return this.replaceRoute('shop.select', index); } } }); // app/routes/shop/index.js export default Ember.Route.extend({ beforeModel() { return this.replaceWith('shop.select', 0); } }); // app/routes/shop/select.js export default Ember.Route.extend({ setupController(_, { index }) { this._super(...arguments); const parentController = this.controllerFor('shop'), itemsLength = parentController.get('cart.items.length'); index = parseInt(index, 10); if (Number.isNaN(index) || index < 0 || index >= itemsLength) { return this.replaceWith('shop.index'); } parentController.set('selectedItemIndex', index); }, model: _ => _ }); ```
pixelhandler commented 5 years ago

@mwpastore thanks for the update we'll close this out. Have a great weekend!

pixelhandler commented 5 years ago

@locks if you want please reopen a documentation issue to note this on the setupController (see the notes in dropdowns by @mwpastore as examples.

pixelhandler commented 5 years ago

@mwpastore Sounds like we could follow up with a post on the forum on https://discuss.emberjs.com/ with your solution

Kerrick commented 5 years ago

Wait so this issue got closed with an alternate solution to work around the bug it reports, and that solution involves setting a property in a computed getter?

adong commented 5 years ago

I'm curious if there's update on this.

I'm using Ember.run.next inside Route's setupController