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

Ensure initial transition can be aborted then retried. #213

Merged
merged 2 commits into from
May 24, 2017

Conversation

rwjblue
Copy link
Collaborator

@rwjblue rwjblue commented May 18, 2017

Attempting to recreate the scenario identified in emberjs/ember.js#15190.

@rwjblue rwjblue force-pushed the abort-initial-transition-retry-method branch from cf8a0b7 to f1a83bf Compare May 18, 2017 17:14
@rwjblue rwjblue force-pushed the abort-initial-transition-retry-method branch from f1a83bf to 7ba0712 Compare May 18, 2017 20:32
When the initial transition is aborted and subsequently retried,
the `urlMethod` of `null` was being inherited. This meant that upon
retry, the URL would never be updated.
@rwjblue rwjblue changed the title [WIP] Add test for initial transition being aborted then retried. Ensure initial transition can be aborted then retried. May 18, 2017
@rwjblue
Copy link
Collaborator Author

rwjblue commented May 18, 2017

@alexspeller @cibernox - This fixes emberjs/ember.js#15190 (if y'all are curious to the root cause).

// inherited for a new transition because then
// the url would not update even though it should
if (this.urlMethod !== null) {
newTransition.method(this.urlMethod);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused actually. I thought that the urlMethod could be either update or replace. Under what circumstances is it null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cibernox - It is null only for the very first initial transition. The reason for this, is that the browser has already updated the window.location and we are basically just telling the system to update its internal state to match the current URL. The docs for Router#handleURL explain it a bit also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FWIW, I think it may be better to just add another flag for this to the Transition object. That way the urlMethod doesn't need to play double duty, and its much more obvious what is going on...

Copy link
Contributor

@alexspeller alexspeller left a comment

Choose a reason for hiding this comment

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

This looks good to me, I didn't realize urlMethod could be null, but now that I do, this makes total sense and is a good fix

@rwjblue rwjblue merged commit b6e5954 into tildeio:master May 24, 2017
@rwjblue rwjblue deleted the abort-initial-transition-retry-method branch May 24, 2017 21:00
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.

3 participants