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.

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

"merge params" option #5007

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

blazmrak
Copy link

@blazmrak blazmrak commented Oct 2, 2022

Added "merge params" option to application as discussed in #5006.

Note: I am not familiar with the suite, so I copy pasted mergeParams tests and adapted them a bit to the application where needed, but I just wanted to make sure that the behavior of the application is the same as the router. The tests should look the same for both however... Would it make sense to provide [Router({mergerParams: true}), app.handle] and run the same tests for them, or is it preferable to have these duplicated?

@dougwilson
Copy link
Contributor

The tests should look the same for both however... Would it make sense to provide [Router({mergerParams: true}), app.handle] and run the same tests for them, or is it preferable to have these duplicated?

It's perfectly fine how you have it and we typically keep them duplicated, as we ended up having lots of accidental regressions in the past when trying to be clever in the test suite and consolidate them with various logic. I think there are still a couple places left like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants