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

[DOC release] Add note about router service event and property update… #18738

Merged
merged 1 commit into from
Feb 22, 2020

Conversation

xg-wang
Copy link
Contributor

@xg-wang xg-wang commented Feb 12, 2020

… timing

Also adding a bunch of hook/event assertions in tests to clearly state the supported timing order.

Related #18665

@locks locks requested a review from chadhietala February 12, 2020 08:15
@xg-wang
Copy link
Contributor Author

xg-wang commented Feb 13, 2020

I'm not entirely sure if it should be documented as public for exact timing order. In fact I would like to deprecate Route's didTransition action, then make it public saying currentURL etc updates after model hooks and before the final routeDidChange event during mid-transition.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Overall, I think this is great! Thanks for working on it!

Comment on lines 442 to 444
The exact timing to fire `routeDidChange` is after the Route's
[didTransition](/ember/release/classes/Route/events/didTransition?anchor=didTransition)
action fired.
Copy link
Member

Choose a reason for hiding this comment

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

I think that it is good to explicitly state this, but I'd prefer to tweak the verbiage here a bit. Maybe something:

`routeDidChange` will be called after any `Route`'s [didTransition](/ember/release/classes/Route/events/didTransition?anchor=didTransition) action has been fired.

Comment on lines 476 to 477
This property is updated after Route's [didTransition](/ember/release/classes/Route/events/didTransition?anchor=didTransition)
action and before `routeDidChange` being fired.
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I would actually prefer to change these to actually be based on routeDidChange.

@rwjblue
Copy link
Member

rwjblue commented Feb 18, 2020

in fact I would like to deprecate Route's didTransition action

Not sure I agree here. While I do much prefer to encourage usage of routeDidChange over action usage, it is actually pretty annoying to write the code for an individual route to subscribe to routeDidChange and determine if it was the target of the transition or if it was above or below the "pivot point" (this code is the kind of thing I am referring to here). Until we have a good answer here, I don't feel good about deprecating the actions...

@xg-wang
Copy link
Contributor Author

xg-wang commented Feb 19, 2020

Forgot to mention when I said I would like to deprecate didTransition, a similar routeDidChange which bubbles similar to the action event needs be added.

But I think you are right at a better solution may be to have something to determine if it was the target of the transition or if it was above or below the "pivot point".

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thank you!

@rwjblue rwjblue merged commit ac45529 into emberjs:master Feb 22, 2020
@xg-wang xg-wang deleted the router-doc branch February 24, 2020 23:24
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.

2 participants