From 7802351d0528e246e22d1b0dd835441ab3513140 Mon Sep 17 00:00:00 2001 From: raytiley Date: Wed, 6 Apr 2016 21:26:28 -0400 Subject: [PATCH 1/2] Normalize query params when merging in from active transition This merges in queryParams from an active transition in such a way that `prepareQueryParams` won't duplicate them. It also won't merge in the qp if it is already int he qp changed list, for example if it's already been set during route setup by a call to `controller.set('qpProp', 'someValue')`. Fixing these things with my current test jsbin also recreated another issue that has been biting people and that is that a call to `transitionTo` creates an aborted transition that calls `didTransition` with an empty set of handlerInfos. This would cause the `updatePaths` method to fail since it couldn't calculate the paths without the handler infos. We now bail early from `updatePaths` it is called with an empty array of handlerInfos. I think this is fine because the aborted transition is followed by a successful transition that will update the paths appropriatly. This gets the active queryParams and the provided using the same key `controller:prop` so that the calls to `assign` override as expected. This fixes issues where the QP doesn't update properly on refresh because the merged value conflicts with the provided value. --- packages/ember-routing/lib/system/router.js | 54 ++++++++++++++++++- .../ember/tests/routing/query_params_test.js | 37 +++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/packages/ember-routing/lib/system/router.js b/packages/ember-routing/lib/system/router.js index 9ccec52708b..bd535edf849 100644 --- a/packages/ember-routing/lib/system/router.js +++ b/packages/ember-routing/lib/system/router.js @@ -723,6 +723,8 @@ const EmberRouter = EmberObject.extend(Evented, { assign(queryParams, this.router.activeTransition.queryParams); } + this._processActiveTransitionQueryParams(targetRouteName, models, queryParams, _queryParams); + assign(queryParams, _queryParams); this._prepareQueryParams(targetRouteName, models, queryParams); @@ -734,6 +736,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 query params so that we can create one object + // that represetns both pased in query params and ones that arent' changed + // from the actice 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 +801,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 +841,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 +1098,8 @@ function calculatePostTransitionState(emberRouter, leafRouteName, contexts) { function updatePaths(router) { let infos = router.router.currentHandlerInfos; + if (infos.length === 0) { return; } + let path = EmberRouter._routePath(infos); let currentRouteName = infos[infos.length - 1].name; diff --git a/packages/ember/tests/routing/query_params_test.js b/packages/ember/tests/routing/query_params_test.js index 1c8c9460e51..0d1e83722cc 100644 --- a/packages/ember/tests/routing/query_params_test.js +++ b/packages/ember/tests/routing/query_params_test.js @@ -2490,6 +2490,43 @@ 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() { + Ember.TEMPLATES.application = compile( + '' + + '{{foo}}' + + '{{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({ From f693865338abe24194f586a2bcb929821d172bd9 Mon Sep 17 00:00:00 2001 From: Igor Terzic Date: Wed, 20 Apr 2016 18:35:35 -0700 Subject: [PATCH 2/2] Add renetrant test for query params changes --- packages/ember-routing/lib/system/router.js | 13 +- .../ember/tests/routing/query_params_test.js | 140 +++++++++++++++++- 2 files changed, 139 insertions(+), 14 deletions(-) diff --git a/packages/ember-routing/lib/system/router.js b/packages/ember-routing/lib/system/router.js index bd535edf849..f71933e5836 100644 --- a/packages/ember-routing/lib/system/router.js +++ b/packages/ember-routing/lib/system/router.js @@ -717,11 +717,6 @@ 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); @@ -738,7 +733,7 @@ const EmberRouter = EmberObject.extend(Evented, { _processActiveTransitionQueryParams(targetRouteName, models, queryParams, _queryParams) { // merge in any queryParams from the active transition which could include - // queryparams from the url on initial load. + // queryParams from the url on initial load. if (!this.router.activeTransition) { return; } var unchangedQPs = {}; @@ -749,9 +744,9 @@ const EmberRouter = EmberObject.extend(Evented, { } } - // We need to fully scope query params so that we can create one object - // that represetns both pased in query params and ones that arent' changed - // from the actice transition + // 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); diff --git a/packages/ember/tests/routing/query_params_test.js b/packages/ember/tests/routing/query_params_test.js index 0d1e83722cc..0489c915668 100644 --- a/packages/ember/tests/routing/query_params_test.js +++ b/packages/ember/tests/routing/query_params_test.js @@ -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'; @@ -2491,11 +2492,13 @@ if (isEnabled('ember-routing-route-configured-query-params')) { }); QUnit.test('queryParams are updated when a controller property is set and the route is refreshed. Issue #13263 ', function() { - Ember.TEMPLATES.application = compile( - '' + - '{{foo}}' + - '{{outlet}}' - ); + setTemplates({ + application: compile( + '' + + '{{foo}}' + + '{{outlet}}' + ) + }); App.ApplicationController = Controller.extend({ queryParams: ['foo'], foo: 1, @@ -3221,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() {