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

fix: qp-only transition during initial transition #307

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

Conversation

buschtoens
Copy link

Seems to fix emberjs/ember.js#18577.

image

@jelhan
Copy link

jelhan commented Oct 29, 2020

Please see my comment here: emberjs/ember.js#18577 (comment) Not sure if this is another bug or if this fix does not address the root cause.

@@ -120,7 +120,7 @@ export default abstract class Router<T extends Route> {
// perform a URL update at the end. This gives
// the user the ability to set the url update
// method (default is replaceState).
let newTransition = new InternalTransition(this, undefined, undefined);
let newTransition = new InternalTransition(this, undefined, newState);
newTransition.queryParamsOnly = true;
Copy link
Author

@buschtoens buschtoens Oct 30, 2020

Choose a reason for hiding this comment

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

Suggested change
newTransition.queryParamsOnly = true;
newTransition.queryParamsOnly = true;
this.setupContexts(newState, newTransition);

Some other tests started breaking with the undefined -> newState change in L123, because this apparently leaks through Routes which haven't yet been passed through setupContexts(...) and thus did not have their setup(...) hook called.

Ultimately this causes Route#finalizeQueryParamChange to fail, as it expects route.controller to be defined.

@rwjblue
Copy link
Collaborator

rwjblue commented Nov 6, 2020

This fix looks good conceptually, but we need to fixup the tests and add a specific test for the scenario described in emberjs/ember.js#18577

@wagenet
Copy link
Collaborator

wagenet commented Feb 9, 2022

@buschtoens status?

@Mikek2252
Copy link

@buschtoens @rwjblue Is this solution still good? if so is there anything i can do to help get this merged? Happy to write the test for it if i can be pushed in the correct general direction, im not overly familiar with this code.

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