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.

Sign up for GitHub

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

Jump to bottom

[Feature Router Service] routeWillChange & routeDidChange #17025

Merged
merged 4 commits into from
Oct 3, 2018
Merged

[Feature Router Service] routeWillChange & routeDidChange #17025

merged 4 commits into from
Oct 3, 2018

Conversation

chadhietala
Copy link
Contributor

@chadhietala chadhietala commented Oct 1, 2018 •
edited
Loading

This adds the routeWillChange & routeDidChange events. It also exposes to and from fields on the transition object. These properties point to a RouteInfo object that looks like:

interface RouteInfo {
  name: string;
  localName: string;
  params: Dict<unknown>;
  paramNames: string[];
  queryParams: Dict<unknown>;
  parent: RouteInfo | null;
  child: RouteInfo | null;
  find((routeInfo: RouteInfo, i: number, routeInfos: RouteInfo[]) => boolean, thisArg: any): RouteInfo : undefined;
}

This also deprecates didTransition and willTransition.

This should land after #17007

@chadhietala chadhietala mentioned this pull request Oct 1, 2018
Closed
16 tasks
rwjblue
rwjblue reviewed Oct 2, 2018
View reviewed changes
packages/@ember/-internals/routing/lib/services/router.ts Outdated
@@ -148,7 +150,17 @@ export default class RouterService extends Service {
}
}

RouterService.reopen({
RouterService.reopen(Evented, {
init() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the init up into the main class body, and leave the .reopen(Evented) here?

packages/@ember/-internals/routing/lib/system/route.ts Outdated
@@ -329,7 +329,7 @@ class Route extends EmberObject implements IRoute {
/**
@private

@method _reset
@method _internalReset
@since 1.7.0
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 think this @since is also wrong

packages/@ember/-internals/routing/lib/system/router.ts Outdated
false,
{
id: 'deprecate-router-events',
until: '3.9.0',
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be 4.0.0, didTransition and willTransition were public API's.

packages/@ember/-internals/routing/lib/system/router.ts Outdated
false,
{
id: 'deprecate-router-events',
until: '3.9.0',
Copy link
Member

Choose a reason for hiding this comment

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

until: '4.0.0'

packages/@ember/-internals/routing/lib/system/router.ts Outdated
@@ -1793,4 +1823,37 @@ EmberRouter.reopen(Evented, {
}),
});

if (EMBER_ROUTING_ROUTER_SERVICE) {
if (ROUTER_EVENTS) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you collapse these two conditionals?

if (EMBER_ROUTING_ROUTER_SERVICE && ROUTER_EVENTS) {

packages/@ember/-internals/routing/lib/system/router.ts
@@ -257,6 +259,34 @@ class EmberRouter extends EmberObject {
return triggerEvent.bind(router)(routeInfos, ignoreFailure, name, args);
}

routeWillChange(transition: Transition) {
if (EMBER_ROUTING_ROUTER_SERVICE) {
router.trigger('routeWillChange', transition);
Copy link
Member

Choose a reason for hiding this comment

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

The new routeWillChange / routeDidChange events need documentation. Can you add that to the meta issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the documentation for this. Not sure if its good enough though. I will add it to the list. Also need to document to and from fields on the transition. Need to also mark Transition as public.

@chadhietala chadhietala force-pushed the to-from branch 2 times, most recently from 5447d27 to 9e31a44 Compare October 3, 2018 13:43
@chadhietala
[Feature Router Service] routeWillChange & routeDidChange
c5a2913
@chadhietala
add docs and more deprecations
d8ee552
@chadhietala
Remove old Router Service FF
0f95c77 
This removes the FF around all the router service features that were
go'd earlier this year. It also reuses the same flag to guard the new
features that we are adding.
@chadhietala chadhietala merged commit 7b00f7b into master Oct 3, 2018
@chadhietala chadhietala deleted the to-from branch October 3, 2018 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chadhietala @rwjblue