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

[BUGFIX beta] fixes transitionTo failing when called in beforeModel hook #13426

Conversation

mattmarcum
Copy link

This is a fix for 11563.

This bug is caused when a queryParamOnly transitionTo is called in the beforeModel hook.

There's a couple of problems here. One is that route.finalizeQueryParams expects the route's controller to already be setup, but it isn't until the route.setup hook is called. The problem is that route.setup is not called until after the beforeModel hook is run.

The solution I came up with is to extract out the relevant portions of the controller setup from route.setup and then check during route.finalizeQueryParams to see if the controller has been added, and if not, add it to the route.

There was another problem with updatePaths on the router not exiting out quick enough. @raytiley has a similar fix in #13273 but in my case the infos param was not even an array.

@mattmarcum
Copy link
Author

All the tests pass locally I promise! I think the build is failing because of a deprecated package in npm.

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2016

I think its fixed (we cleared the Travis cache), going to restart the build...

var changes = router._qpUpdates;
var replaceUrl;

stashParamNames(router, handlerInfos);
if (!handlerInfos.length) {
Copy link
Member

Choose a reason for hiding this comment

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

So we don't want to stashParamNames if there are no handlerInfos? This is to ensure that #setup has been called first, right? If so, is there a better way to check this (or add a comment that explains why)?

@raytiley
Copy link
Contributor

This looks good to me with the caveat that my eyes start to bleed when staring at at the router code for too long.

This will be great cause this issue is what I was blocked on for #13273. Once this is in I'll finish up some of the other tests and then query params not bug people so much for a while :)

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2016

I think that this augments #13273 pretty well. I had a couple of minor nitpicks that I'd like to see addressed, but other than that I'm pretty happy with this. Thank you for taking the time to work on it!

@mattmarcum mattmarcum force-pushed the bugfix-11563-transition-in-beforeModel-throws-error branch from 4e084d9 to 4205d59 Compare April 28, 2016 21:05
@mattmarcum
Copy link
Author

@rwjblue fixed those. I used transition.queryParamsOnly when deciding whether to call stashParamNames. The queryParamsOnly flag gets set when the no-op transition gets created (hence the reason the handlerInfos are empty). I think it makes it more clear.

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2016

Awesome, thank you! Will merge once Travis agrees....

@mattmarcum
Copy link
Author

Oops, looks like that last change broke some other tests...I'm going to revert back and just add a comment instead.

@mattmarcum mattmarcum force-pushed the bugfix-11563-transition-in-beforeModel-throws-error branch from 4205d59 to 600ed95 Compare April 28, 2016 21:30
@mattmarcum mattmarcum force-pushed the bugfix-11563-transition-in-beforeModel-throws-error branch from 600ed95 to 17eac20 Compare April 28, 2016 21:45
Matt Marcum and others added 2 commits May 2, 2016 14:58
@homu
Copy link
Contributor

homu commented May 6, 2016

☔ The latest upstream changes (presumably b125ff5) made this pull request unmergeable. Please resolve the merge conflicts.

…tion-in-beforeModel-throws-error

Conflicts:
	packages/ember/tests/routing/query_params_test.js
@homu
Copy link
Contributor

homu commented May 25, 2016

☔ The latest upstream changes (presumably ceb2b15) made this pull request unmergeable. Please resolve the merge conflicts.

@mattmarcum
Copy link
Author

After looking at this code again, I'm thinking that a better solution would need to touch the router.js code too. I'm going to close this pr and think about a better solution. Any one else, feel free to try and fix this.

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

Successfully merging this pull request may close these issues.

4 participants