Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX beta] exempt routes that share a controller from duplicate assertion #14791

Merged
merged 1 commit into from Jan 11, 2017
Merged

[BUGFIX beta] exempt routes that share a controller from duplicate assertion #14791

merged 1 commit into from Jan 11, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jan 5, 2017

start of fix for #14560

@ghost ghost changed the title WIP: make check more detailed [BUGFIX 14560] exempt routes that share a controller from duplicate assertion Jan 6, 2017
@ghost
Copy link
Author

ghost commented Jan 6, 2017

@trentmwillis mind reviewing to ensure fix is in right direction?
Ideally, a fix for this could be merged since this issue is blocking our upgrade. I figured a PR with the beginnings here is more sensible then writing some workaround specific to our application.

trentmwillis
trentmwillis suggested changes Jan 9, 2017
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

packages/ember-routing/lib/system/router.js

if (qpsByUrlKey[urlKey]) {
if (qpOther && (qpOther.hasOwnProperty('controllerName') && qpOther.controllerName !== qp.controllerName)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

packages/ember-routing/tests/system/router_test.js
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ghost
Copy link
Author

ghost commented Jan 9, 2017 •
edited by ghost
Loading

thanks for the feedback; will be updating here within the morning.
I removed the assertion verifying the caching in _queryParamsFor. IMHO, such verification only makes sense in a unit test and didn't see a clear way to implement it using the existing integration tests.

trentmwillis
trentmwillis suggested changes Jan 9, 2017
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small thing, otherwise seems good to me. Will need a review from an actual maintainer though.

packages/ember/tests/routing/query_params_test/overlapping_query_params_test.js
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Need a newline before and after the test block

@ghost ghost changed the title [BUGFIX 14560] exempt routes that share a controller from duplicate assertion [BUGFIX release] exempt routes that share a controller from duplicate assertion Jan 9, 2017
trentmwillis
trentmwillis approved these changes Jan 10, 2017
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@rwjblue
Copy link
Member

rwjblue commented Jan 11, 2017

OK, I squashed, rebased, and updated commit message. Will merge once Travis is green...

@ghost
Copy link
Author

ghost commented Jan 11, 2017

thanks @trentmwillis and @rwjblue !

@rwjblue rwjblue changed the title [BUGFIX release] exempt routes that share a controller from duplicate assertion [BUGFIX beta] exempt routes that share a controller from duplicate assertion Jan 11, 2017
@rwjblue rwjblue merged commit 3a6bdc7 into emberjs:master Jan 11, 2017
@ghost ghost deleted the fix-14560 branch January 11, 2017 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rwjblue @trentmwillis @eflanagan0