-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BUGFIX] - Merge query params from previous transition into active transition #13273
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -717,11 +717,8 @@ const EmberRouter = EmberObject.extend(Evented, { | |
assert(`The route ${targetRouteName} was not found`, targetRouteName && this.router.hasRoute(targetRouteName)); | ||
|
||
let queryParams = {}; | ||
// merge in any queryParams from the active transition which could include | ||
// queryparams from the url on initial load. | ||
if (this.router.activeTransition) { | ||
assign(queryParams, this.router.activeTransition.queryParams); | ||
} | ||
|
||
this._processActiveTransitionQueryParams(targetRouteName, models, queryParams, _queryParams); | ||
|
||
assign(queryParams, _queryParams); | ||
this._prepareQueryParams(targetRouteName, models, queryParams); | ||
|
@@ -734,6 +731,27 @@ const EmberRouter = EmberObject.extend(Evented, { | |
return transition; | ||
}, | ||
|
||
_processActiveTransitionQueryParams(targetRouteName, models, queryParams, _queryParams) { | ||
// merge in any queryParams from the active transition which could include | ||
// queryParams from the url on initial load. | ||
if (!this.router.activeTransition) { return; } | ||
|
||
var unchangedQPs = {}; | ||
var qpUpdates = this._qpUpdates || {}; | ||
for (var key in this.router.activeTransition.queryParams) { | ||
if (!qpUpdates[key]) { | ||
unchangedQPs[key] = this.router.activeTransition.queryParams[key]; | ||
} | ||
} | ||
|
||
// We need to fully scope queryParams so that we can create one object | ||
// that represents both pased in queryParams and ones that aren't changed | ||
// from the active transition. | ||
this._fullyScopeQueryParams(targetRouteName, models, _queryParams); | ||
this._fullyScopeQueryParams(targetRouteName, models, unchangedQPs); | ||
assign(queryParams, unchangedQPs); | ||
}, | ||
|
||
_prepareQueryParams(targetRouteName, models, queryParams) { | ||
this._hydrateUnsuppliedQueryParams(targetRouteName, models, queryParams); | ||
this._serializeQueryParams(targetRouteName, queryParams); | ||
|
@@ -778,6 +796,32 @@ const EmberRouter = EmberObject.extend(Evented, { | |
}; | ||
}, | ||
|
||
_fullyScopeQueryParams(leafRouteName, contexts, queryParams) { | ||
var state = calculatePostTransitionState(this, leafRouteName, contexts); | ||
var handlerInfos = state.handlerInfos; | ||
stashParamNames(this, handlerInfos); | ||
|
||
for (var i = 0, len = handlerInfos.length; i < len; ++i) { | ||
var route = handlerInfos[i].handler; | ||
var qpMeta = get(route, '_qp'); | ||
|
||
for (var j = 0, qpLen = qpMeta.qps.length; j < qpLen; ++j) { | ||
var qp = qpMeta.qps[j]; | ||
|
||
var presentProp = qp.prop in queryParams && qp.prop || | ||
qp.scopedPropertyName in queryParams && qp.scopedPropertyName || | ||
qp.urlKey in queryParams && qp.urlKey; | ||
|
||
if (presentProp) { | ||
if (presentProp !== qp.scopedPropertyName) { | ||
queryParams[qp.scopedPropertyName] = queryParams[presentProp]; | ||
delete queryParams[presentProp]; | ||
} | ||
} | ||
} | ||
} | ||
}, | ||
|
||
_hydrateUnsuppliedQueryParams(leafRouteName, contexts, queryParams) { | ||
let state = calculatePostTransitionState(this, leafRouteName, contexts); | ||
let handlerInfos = state.handlerInfos; | ||
|
@@ -792,7 +836,8 @@ const EmberRouter = EmberObject.extend(Evented, { | |
let qp = qpMeta.qps[j]; | ||
|
||
let presentProp = qp.prop in queryParams && qp.prop || | ||
qp.scopedPropertyName in queryParams && qp.scopedPropertyName; | ||
qp.scopedPropertyName in queryParams && qp.scopedPropertyName || | ||
qp.urlKey in queryParams && qp.urlKey; | ||
|
||
if (presentProp) { | ||
if (presentProp !== qp.scopedPropertyName) { | ||
|
@@ -1048,6 +1093,8 @@ function calculatePostTransitionState(emberRouter, leafRouteName, contexts) { | |
|
||
function updatePaths(router) { | ||
let infos = router.router.currentHandlerInfos; | ||
if (infos.length === 0) { return; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm running into a similar problem but in my case
|
||
|
||
let path = EmberRouter._routePath(infos); | ||
let currentRouteName = infos[infos.length - 1].name; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import Controller from 'ember-runtime/controllers/controller'; | ||
import RSVP from 'ember-runtime/ext/rsvp'; | ||
import Route from 'ember-routing/system/route'; | ||
import run from 'ember-metal/run_loop'; | ||
import get from 'ember-metal/property_get'; | ||
|
@@ -2490,6 +2491,45 @@ if (isEnabled('ember-routing-route-configured-query-params')) { | |
bootApplication(); | ||
}); | ||
|
||
QUnit.test('queryParams are updated when a controller property is set and the route is refreshed. Issue #13263 ', function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test looks good! Seems to me that this test will fail with "Uncaught Error, Assertion Failed: You're not allowed to have more than one controller property map to the same query param key…" without the code to fix. |
||
setTemplates({ | ||
application: compile( | ||
'<button id="test-button" {{action \'increment\'}}>Increment</button>' + | ||
'<span id="test-value">{{foo}}</span>' + | ||
'{{outlet}}' | ||
) | ||
}); | ||
App.ApplicationController = Controller.extend({ | ||
queryParams: ['foo'], | ||
foo: 1, | ||
actions: { | ||
increment: function() { | ||
this.incrementProperty('foo'); | ||
this.send('refreshRoute'); | ||
} | ||
} | ||
}); | ||
|
||
App.ApplicationRoute = Route.extend({ | ||
actions: { | ||
refreshRoute: function() { | ||
this.refresh(); | ||
} | ||
} | ||
}); | ||
|
||
startingURL = '/'; | ||
bootApplication(); | ||
equal(jQuery('#test-value').text().trim(), '1'); | ||
equal(router.get('location.path'), '/', 'url is correct'); | ||
run(jQuery('#test-button'), 'click'); | ||
equal(jQuery('#test-value').text().trim(), '2'); | ||
equal(router.get('location.path'), '/?foo=2', 'url is correct'); | ||
run(jQuery('#test-button'), 'click'); | ||
equal(jQuery('#test-value').text().trim(), '3'); | ||
equal(router.get('location.path'), '/?foo=3', 'url is correct'); | ||
}); | ||
|
||
QUnit.test('Use Ember.get to retrieve query params \'refreshModel\' configuration', function() { | ||
expect(6); | ||
App.ApplicationController = Controller.extend({ | ||
|
@@ -3184,6 +3224,133 @@ if (isEnabled('ember-routing-route-configured-query-params')) { | |
let controller = container.lookup('controller:example'); | ||
equal(get(controller, 'foo'), undefined); | ||
}); | ||
|
||
QUnit.test('when refreshModel is true and loading action returns false, model hook will rerun when QPs change even if previous did not finish', function() { | ||
expect(6); | ||
|
||
var appModelCount = 0; | ||
var promiseResolve; | ||
|
||
App.ApplicationRoute = Route.extend({ | ||
queryParams: { | ||
'appomg': { | ||
defaultValue: 'applol' | ||
} | ||
}, | ||
model(params) { | ||
appModelCount++; | ||
} | ||
}); | ||
|
||
App.IndexController = Controller.extend({ | ||
queryParams: ['omg'] | ||
// uncommon to not support default value, but should assume undefined. | ||
}); | ||
|
||
var indexModelCount = 0; | ||
App.IndexRoute = Route.extend({ | ||
queryParams: { | ||
omg: { | ||
refreshModel: true | ||
} | ||
}, | ||
actions: { | ||
loading: function() { | ||
return false; | ||
} | ||
}, | ||
model(params) { | ||
indexModelCount++; | ||
if (indexModelCount === 2) { | ||
deepEqual(params, { omg: 'lex' }); | ||
return new RSVP.Promise(function(resolve) { | ||
promiseResolve = resolve; | ||
return; | ||
}); | ||
} else if (indexModelCount === 3) { | ||
deepEqual(params, { omg: 'hello' }, 'Model hook reruns even if the previous one didnt finish'); | ||
} | ||
} | ||
}); | ||
|
||
bootApplication(); | ||
|
||
equal(indexModelCount, 1); | ||
|
||
var indexController = container.lookup('controller:index'); | ||
setAndFlush(indexController, 'omg', 'lex'); | ||
equal(indexModelCount, 2); | ||
|
||
setAndFlush(indexController, 'omg', 'hello'); | ||
equal(indexModelCount, 3); | ||
run(function() { | ||
promiseResolve(); | ||
}); | ||
equal(get(indexController, 'omg'), 'hello', 'At the end last value prevails'); | ||
}); | ||
|
||
QUnit.test('when refreshModel is true and loading action does not return false, model hook will not rerun when QPs change even if previous did not finish', function() { | ||
expect(7); | ||
|
||
var appModelCount = 0; | ||
var promiseResolve; | ||
|
||
App.ApplicationRoute = Route.extend({ | ||
queryParams: { | ||
'appomg': { | ||
defaultValue: 'applol' | ||
} | ||
}, | ||
model(params) { | ||
appModelCount++; | ||
} | ||
}); | ||
|
||
App.IndexController = Controller.extend({ | ||
queryParams: ['omg'] | ||
// uncommon to not support default value, but should assume undefined. | ||
}); | ||
|
||
var indexModelCount = 0; | ||
App.IndexRoute = Route.extend({ | ||
queryParams: { | ||
omg: { | ||
refreshModel: true | ||
} | ||
}, | ||
model(params) { | ||
indexModelCount++; | ||
|
||
if (indexModelCount === 2) { | ||
deepEqual(params, { omg: 'lex' }); | ||
return new RSVP.Promise(function(resolve) { | ||
promiseResolve = resolve; | ||
return; | ||
}); | ||
} else if (indexModelCount === 3) { | ||
ok(false, 'shouldnt get here'); | ||
} | ||
} | ||
}); | ||
|
||
bootApplication(); | ||
|
||
equal(appModelCount, 1); | ||
equal(indexModelCount, 1); | ||
|
||
var indexController = container.lookup('controller:index'); | ||
setAndFlush(indexController, 'omg', 'lex'); | ||
|
||
equal(appModelCount, 1); | ||
equal(indexModelCount, 2); | ||
|
||
setAndFlush(indexController, 'omg', 'hello'); | ||
equal(get(indexController, 'omg'), 'hello', ' value was set'); | ||
equal(indexModelCount, 2); | ||
run(function() { | ||
promiseResolve(); | ||
}); | ||
}); | ||
} | ||
|
||
QUnit.test('warn user that routes query params configuration must be an Object, not an Array', function() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please break this down and add more local vars to make this more readable