Fix query param stickiness between models in ember-engines #14794
Conversation
There was a problem hiding this comment.
One small thing, but overall this seems good to me. I think using the route name also is in the direction of moving away from controllers. @rwjblue can you review when you have a chance?
|
|
||
| return this.visit('/blog/category/1?type=news').then(() => { | ||
| let href = this.element.querySelector('a').href; | ||
| assert.equal(href.match(/type=news/), null); |
There was a problem hiding this comment.
Can we change this to just be assert.equal(href, '/blog/category/1337');? It's easier to follow and will give clearer feedback (IMO) if it breaks.
There was a problem hiding this comment.
agreed, I'd much prefer to assert against the value we actually expect...
There was a problem hiding this comment.
I've updated this to use .endsWith since full href include port number. Let me know if you need further change.
There was a problem hiding this comment.
Should be able to use something like:
let { pathname } = this.element.querySelector('a');
assert.equal(pathname, '/blog/category/1337');There was a problem hiding this comment.
endsWith solution didn't work because of not being implemented in phantomjs. pathname solution also didn't work because of ie9 different implementation. I changed this to indexOf to simulate endsWith. Let me know if you guys have better preference.
|
This is looking good! A few specific issues that we should chat through are:
|
rajaalauddin
force-pushed
the
fix-query-param-stickiness
branch
4 times, most recently
from
b4328dd
to
f88f247
Compare
| }); | ||
| } | ||
|
|
||
| ['@test query params in customized controllerName have stickiness by default between model'](assert) { |
There was a problem hiding this comment.
How do we handle scenarios where folks have customized controllerName to specify a controller other than the route's controller is handling their QP's? With this change, I think that use case might break?
@rwjblue I added a test on this and you are correct this change will break queryParam in customized controllerName.
There was a problem hiding this comment.
I stand corrected - this change doesn't break custom controllerName case. There was an error in the way i register the route causing the test to fail. I've updated the setup and now it's passing.
rajaalauddin
force-pushed
the
fix-query-param-stickiness
branch
from
f88f247
to
9e28908
Compare
@rwjblue I added test to cover this scenario and it's passing. Please verify if that is what you have in mind. thanks |
See #14788
Summary:
In ember-engines, query param is sticky between different models (see generated link-to). This is not the behavior outside engine and as specified https://guides.emberjs.com/v2.10.0/routing/query-params/#toc_sticky-query-param-values
This is happening because they share the same cacheKey when controller is used as the prefix.