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

Add failing test for query param scoping #17451

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wagenet
Copy link
Member

@wagenet wagenet commented Jan 8, 2019

The issue here is that for query param scoping, it is assumed that the only thing that matters are the URL params on the final route. However, it is entirely possible (as shown in the test) that this might not be sufficient to determine uniqueness. It's not clear to me how best to fix this since in some cases this assumption might be correct (e.g. the params are just the ids of Ember Data records) but in others it may not be (e.g. the final route's params aren't sufficient to determine the model).

It should also be noted that the guides say that the query params are tied to the model. This is definitely not correct as we see here. They are tied to the url params of the last route.

@wagenet wagenet force-pushed the query-param-scoping branch 2 times, most recently from 88dd2ad to b90b4b7 Compare January 8, 2019 22:31
@chadhietala
Copy link
Contributor

Is this a regression as of 3.6?

@wagenet
Copy link
Member Author

wagenet commented Jan 9, 2019

@chadhietala I believe it's been present since at least 3.4.6 as we discovered it in our app which is stuck on 3.4 for unrelated reasons.

@wagenet wagenet force-pushed the query-param-scoping branch 2 times, most recently from 4b426f7 to 2ed3a42 Compare March 8, 2021 22:33
@locks
Copy link
Contributor

locks commented Feb 5, 2022

@wagenet is this still relevant?

@wagenet wagenet force-pushed the query-param-scoping branch from 2ed3a42 to 5b39a38 Compare February 7, 2022 17:05
@wagenet
Copy link
Member Author

wagenet commented Feb 7, 2022

@locks I just rebased. Looks like CI still fails.

@kategengler
Copy link
Member

Is this still relevant?

@wagenet wagenet force-pushed the query-param-scoping branch from 5b39a38 to 06da75b Compare January 10, 2024 22:00
@wagenet
Copy link
Member Author

wagenet commented Jan 10, 2024

Rebased again just now.

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

Successfully merging this pull request may close these issues.

4 participants