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 release] Ensure handleURL called after setURL in visit helper #11317

Merged
merged 1 commit into from Jun 2, 2015

Conversation

jaswilli
Copy link
Contributor

@jaswilli jaswilli commented Jun 1, 2015

There's a regression in 1.12.1 where if you visit() a URL that has a transition (this.transitionTo() in a beforeModel hook) after already visiting some other URL that handleURL won't be invoked with the correct context and currentURL() will yield the "pre-transition" URL.

Here's a failing jsbin for 1.12.1: http://emberjs.jsbin.com/nafakuz/5/edit

And a working jsbin on 1.11.3: http://emberjs.jsbin.com/tosacu/2/edit

@@ -59,14 +59,13 @@ function visit(app, url) {
var router = app.__container__.lookup('router:main');
app.boot().then(function() {
router.location.setURL(url);
run(app.__deprecatedInstance__, 'handleURL', url);
Copy link
Member

Choose a reason for hiding this comment

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

Lets tweak this slightly so it only runs in the else case below (via a guard or something). Otherwise, we end up double routing on the first visit call (a default app boot would have transitioned into the initial URL then we call handleURL to transition again).

@rwjblue
Copy link
Member

rwjblue commented Jun 1, 2015

Thank you for tracking down and fixing!! I left a small inline comment/tweak, but should be good to go once that is fixed...

@rwjblue
Copy link
Member

rwjblue commented Jun 1, 2015

👍 - LGTM

@jaswilli
Copy link
Contributor Author

jaswilli commented Jun 1, 2015

Updated to prevent the double routing. Thanks for the review and feedback! 😄

rwjblue added a commit that referenced this pull request Jun 2, 2015
[BUGFIX release] Ensure handleURL called after setURL in visit helper
@rwjblue rwjblue merged commit 4bd690e into emberjs:master Jun 2, 2015
@jaswilli jaswilli deleted the visit-regression branch June 2, 2015 01:00
@jaswilli
Copy link
Contributor Author

@rwjblue This never made it into 1.12.x and it's not in 1.13.0 either? Is there a problem or did it just get lost in the shuffle?

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2015

It was my mistake :(, I've cherry-picked into stable branch for 1.13.1 release today/tomorrow. I'm very sorry for the oversight.

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.

None yet

2 participants