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

Add a couple of tests to test passing empty queryParams to replaceWith #20042

Closed
wants to merge 1 commit into from
Closed

Add a couple of tests to test passing empty queryParams to replaceWith #20042

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions packages/ember/tests/routing/router_service_test/replaceWith_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,5 +136,29 @@ moduleFor(
assert.deepEqual(this.state, ['/', '/child']);
});
}

['@test RouterService#replaceWith with empty query params'](assert) {
assert.expect(1);

return this.visit('/')
.then(() => {
return this.routerService.replaceWith('parent.child', { queryParams: {} });
})
.then(() => {
assert.deepEqual(this.state, ['/child']);
});
}

['@test RouterService#replaceWith with `undefined` in query params'](assert) {
assert.expect(1);

return this.visit('/')
.then(() => {
return this.routerService.replaceWith('parent.child', { queryParams: undefined });
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough! I'll look into it more next week.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, empty query params should work at least. I'll see about fixing that.

Copy link
Member

Choose a reason for hiding this comment

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

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.

})
.then(() => {
assert.deepEqual(this.state, ['/child']);
});
}
}
);