[BUGFIX beta] exempt routes that share a controller from duplicate assertion #14791
Conversation
ghost
changed the title
|
@trentmwillis mind reviewing to ensure fix is in right direction? |
|
|
||
| if (qpsByUrlKey[urlKey]) { | ||
| if (qpOther && (qpOther.hasOwnProperty('controllerName') && qpOther.controllerName !== qp.controllerName)) { |
There was a problem hiding this comment.
I'm unsure the hasOwnProperty check here is necessary. Ideally if a collision happens, and the property isn't there then we should do the assertion.
| @@ -255,3 +255,57 @@ QUnit.test('Router#triggerEvent ignores handlers that have not loaded yet', func | |||
|
|
|||
| triggerEvent(handlerInfos, false, ['loading']); | |||
| }); | |||
|
|
|||
| QUnit.test('Router#_queryParamsFor returns the cached leaf handler when present', function(assert) { | |||
There was a problem hiding this comment.
I would rather these tests be done as acceptance-type tests rather than unit tests. They should probably go in this test file which does the other collision checks: https://github.com/emberjs/ember.js/blob/master/packages/ember/tests/routing/query_params_test/overlapping_query_params_test.js
|
thanks for the feedback; will be updating here within the morning. |
| @@ -105,7 +106,20 @@ moduleFor('Query Params - overlapping query param property names', class extends | |||
| this.assertCurrentPath('/parent/child?childPage=3&parentPage=4'); | |||
| }); | |||
| } | |||
| ['@test query params does not error when a query parameter exists for route instances that share a controller'](assert) { | |||
There was a problem hiding this comment.
Nitpick: Need a newline before and after the test block
ghost
changed the title
|
OK, I squashed, rebased, and updated commit message. Will merge once Travis is green... |
|
thanks @trentmwillis and @rwjblue ! |
rwjblue
changed the title
ghost
deleted the
start of fix for #14560