Making router._changedQueryParams / queryParamsDidChange public

[sangm]

Hi,

I maintain state through the controller (using this.controller.set rather than explicitly calling this.transitionTo) so I don't have bunch of calls such as this.transitionTo('foo', 'bar', { queryParams: { ... }}

I ran into an issue where if controller.set('foobar', 'barfoo') happens, I want to reset query params of ['a', 'b'].

I did some looking around, and these are the most relevant code segments I'm interested in
I figured the best way to do something like that is overriding the private hook queryParamsDidChange.

function fireQueryParamDidChange(router, newState, queryParamChangelist) {
  // If queryParams changed trigger event
  if (queryParamChangelist) {

    // This is a little hacky but we need some way of storing
    // changed query params given that no activeTransition
    // is guaranteed to have occurred.
    router._changedQueryParams = queryParamChangelist.all;
    trigger(router, newState.handlerInfos, true, ['queryParamsDidChange', queryParamChangelist.changed, queryParamChangelist.all, queryParamChangelist.removed]);
    router._changedQueryParams = null;
  }
}

That triggers this action

actions: {
      ...
      queryParamsDidChange: function (changed, totalPresent, removed) {
        var qpMap = _emberMetal.get(this, '_qp').map;

        var totalChanged = Object.keys(changed).concat(Object.keys(removed));
        for (var i = 0; i < totalChanged.length; ++i) {
          var qp = qpMap[totalChanged[i]];
          if (qp && _emberMetal.get(this._optionsForQueryParam(qp), 'refreshModel') && this.router.currentState) {
            this.refresh();
          }
        }

        return true;
      },
}

And following refresh leads you to

refresh: function(pivotHandler) {
    var state = this.activeTransition ? this.activeTransition.state : this.state;
    var handlerInfos = state.handlerInfos;
    var params = {};
    for (var i = 0, len = handlerInfos.length; i < len; ++i) {
      var handlerInfo = handlerInfos[i];
      params[handlerInfo.name] = handlerInfo.params || {};
    }

    log(this, "Starting a refresh transition");
    var intent = new NamedTransitionIntent({
      name: handlerInfos[handlerInfos.length - 1].name,
      pivotHandler: pivotHandler || handlerInfos[0].handler,
      contexts: [], // TODO collect contexts...?
      queryParams: this._changedQueryParams || state.queryParams || {}
    });

    return this.transitionByIntent(intent, false);
  },

These are the two options I see:

queryParamsDidChange(changed, total) {
  // this works
  const keysToRemove = ...;
  keysToRemove.forEach(key => {
    if (key in total) {
      delete total[key];
    }
  });
}

or

queryParamsDidChange(changed, total) {
  // this works
  const keysToRemove = ...;
  const newTotal = createNewObjectByRemovingKeys(total, keysToRemove);
  this.router.router._changedQueryParams = newTotal;
}

I would like to keep my functions pure and not mutate the parameter passed in, but accessing the private variable within router is also not ideal.

With that in mind, it would be nice to do something like this

queryParamsDidChange(changed, total, removed) {
 // this works
  const keysToRemove = ...;
  const newTotal = createNewObjectByRemovingKeys(total, keysToRemove);
  return this._super(changed, newTotal, removed);
}

I wouldn't mind making a pull request, but wanted to know if there was already a way to support this, or better way to tackle it. I understand this is a private API, but discussions (emberjs/ember.js#10877) indicate people weren't exactly opposed to making it public.

The change would be trivial, but definitely useful. Changing query params through controller.set is supported, and this problem could be solved by changing all the instances of controller.set to transitionTo, however, I believe this is another valid approach.

@nathanhammond @stefanpenner @trentmwillis

[nathanhammond]

@sangm I'm not sure if this was posted before or after our conversation. 😜

Considering the degree to which we want to change this (very much) and the likelihood of dropping two-way-binding for controller.set (~100%) I can say pretty confidently that we're not going to create additional public API which we would have to maintain just before a significant change.

The future will likely always require direct invocation of transitionTo, though we may add some sugar on top of that to make it simpler to support sticky QPs (model dependent state) and more. That solution likely will not look like two-way-binding and instead more like @knownasilya's https://github.com/knownasilya/ember-query-params

(The implementation for that is based upon the APIs we have now so it's imperfect. We need to put more thought into the final design.)

[sangm]

You can still call transitionTo, but that doesn't change much right?

Whether or not I call transitionTo / controller.set, it might not be a bad idea to expose a hook similar to queryParamsDidChange and allow the user to modify the actual transition intent