Add a couple of tests to test passing empty queryParams to replaceWith
#20042
Conversation
packages/ember/tests/routing/router_service_test/replaceWith_test.js
Outdated
Show resolved
Hide resolved
|
|
||
| return this.visit('/') | ||
| .then(() => { | ||
| return this.routerService.replaceWith('parent.child', { queryParams: undefined }); |
There was a problem hiding this comment.
In this case, this is something we could support but it's also not 100% clear to me that we want to. Can you explain more to me about the use case?
There was a problem hiding this comment.
Well, you can see what I had in my code here. The idea is that users of that component could set in model their own routeComponents and (optionally) queryParameters and the component would transition to that. Users might not add queryParameters which would mean that this call:
this.router.replaceWith(...this.model.routeComponents, { queryParams: this.model.queryParameters })Would become:
this.router.replaceWith(...this.model.routeComponents, { queryParams: undefined })Which used to work on Ember < 4.3.0 but broke on 4.3.0. Not saying it's good, just saying it used to work. And is useful. 😄
There was a problem hiding this comment.
Fair enough! I'll look into it more next week.
There was a problem hiding this comment.
So I played around with this some and it looks like router.js would also need an update to handle this case correctly. In your case, a simple solution is probably this.model.queryParameters || {}. I'm open to more discussion however.
There was a problem hiding this comment.
hm, but as I said, this used to work in the previous Ember version without changes to router.js. But I don't know the current code, of course. Also, note that this.model.queryParameters || {} also doesn't work - check the first test of the two - it tests exactly that (and fails).
There was a problem hiding this comment.
Ah, empty query params should work at least. I'll see about fixing that.
There was a problem hiding this comment.
Also, just because something worked before doesn't mean we guarantee that it will keep working, only if it is something that was clearly documented or widely used to the point where it's basically official. Honestly, this whole API is somewhat awkward so it's not actually clear to me what's most correct. I'm happy to hear another opinion on this.
|
I've involuntarily closed this. I've opened it again as it's still relevant I believe. |
Which should fail. Check this.