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

Query param is sticky for different route model in routeable ember-engine #14788

Closed
rajaalauddin opened this issue Jan 4, 2017 · 5 comments
Closed

Comments

@rajaalauddin
Copy link

I have reproduced this issue in this repo https://github.com/rajaalauddin/ember-engines-demo

Sample steps on how to reproduce the issue:

  1. Create a route inside routable ember engine with dynamic segment. e.g /viewers/:id
  2. Create link-to /viewers/2 for viewers.hbs template
  3. Add query param to viewer controller e.g queryParams: ['active']
  4. Go to /viewers/1?active=true
  5. See how link-to generated on /viewers/2 also has the query param e.g /viewers/2?active=true

These are not the case when done outside engine.

Another developer and i have traced this issue down to how calculateCacheKey is called.

var cacheKey = _emberRoutingUtils.calculateCacheKey(aQp.controllerName, aQp.parts, aQp.values);

Changing the above to:

var cacheKey = _emberRoutingUtils.calculateCacheKey(aQp.route.fullRouteName, aQp.parts, aQp.values);

fixed the problem for us. Let me know if this is the correct solution so that i can create a pull request.

@rwjblue
Copy link
Member

rwjblue commented Jan 5, 2017

Thanks for reporting!

Let me know if this is the correct solution so that i can create a pull request

The solution seems reasonable in general (specifically, we need to ensure that the cache key is built up correctly inside the engine). I think the next step is getting an isolated failing test case.

Please take a look at some of these engine tests tests for an idea of how to set it up. I believe we should be able to add a test in that file that replicates the scenario you describe in the issue description.

@rajaalauddin
Copy link
Author

I have written the failing test case with the fix in one commit below.
rajaalauddin@7672ec9

It was originally failing but now passed after my change

@pixelhandler
Copy link
Contributor

@rajaalauddin perhaps make a PR for the failing test? And the fix too :)

@rajaalauddin
Copy link
Author

@pixelhandler I have created pull request here #14794

@Serabe Serabe added the Has PR label Jan 13, 2017
@Serabe
Copy link
Member

Serabe commented Jan 27, 2017

Closed via #14794

@Serabe Serabe closed this as completed Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants