-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[BUGFIX beta] fixes transitionTo failing when called in beforeModel hook #13426
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
|
All the tests pass locally I promise! I think the build is failing because of a deprecated package in npm. |
|
I think its fixed (we cleared the Travis cache), going to restart the build... |
There was a problem hiding this comment.
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)?
|
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 :) |
|
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! |
4e084d9 to
4205d59
Compare
|
@rwjblue fixed those. I used |
|
Awesome, thank you! Will merge once Travis agrees.... |
|
Oops, looks like that last change broke some other tests...I'm going to revert back and just add a comment instead. |
4205d59 to
600ed95
Compare
600ed95 to
17eac20
Compare
…tion-in-beforeModel-throws-error
…tion-in-beforeModel-throws-error
|
☔ 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
|
☔ The latest upstream changes (presumably ceb2b15) made this pull request unmergeable. Please resolve the merge conflicts. |
|
After looking at this code again, I'm thinking that a better solution would need to touch the |
This is a fix for 11563.
This bug is caused when a
queryParamOnlytransitionTois called in thebeforeModelhook.There's a couple of problems here. One is that
route.finalizeQueryParamsexpects the route's controller to already be setup, but it isn't until theroute.setuphook is called. The problem is thatroute.setupis not called until after thebeforeModelhook is run.The solution I came up with is to extract out the relevant portions of the controller setup from
route.setupand then check duringroute.finalizeQueryParamsto see if the controller has been added, and if not, add it to the route.There was another problem with
updatePathson the router not exiting out quick enough. @raytiley has a similar fix in #13273 but in my case theinfosparam was not even an array.