emberjs / emberjs/router.js

Making `router._changedQueryParams` / `queryParamsDidChange` public

Open
#196 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
1.3k
Forks
159
PR merge metrics
No merged PRs in 30d

Description

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 (https://github.com/emberjs/ember.js/issues/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

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.