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

feat(router): add router-level events for GuardsCheck and Resolve #17601

Merged
merged 1 commit into from Jul 1, 2017

Conversation

Projects
None yet
5 participants
@jasonaden
Copy link
Contributor

commented Jun 18, 2017

This PR adds router-level event hooks so user-land knows when Guards and Resolves start and end for each route navigation. This makes the full lifecycle of events at the router level bigger:

NavigationStart
RoutesRecognized
GuardsCheckStart // new
GuardsCheckEnd // new
ResolveStart // new
ResolveEnd // new
NavigationEnd

With optional events:

RouteConfigLoadStart
RouteConfigLoadEnd
NavigationCancel
NavigationError

This will be especially useful for metrics. Things such as timing how long guards and resolves take on average.

These APIs are experimental and are documented as such.

@googlebot googlebot added the cla: yes label Jun 18, 2017

@jasonaden jasonaden force-pushed the jasonaden:feat_add_router_events branch from 2ba0836 to 8fbf1d3 Jun 18, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 18, 2017

The angular.io preview for 2ba0836 is available here.

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 18, 2017

The angular.io preview for 8fbf1d3 is available here.

@jasonaden jasonaden requested a review from vicb Jun 22, 2017

@@ -137,9 +227,14 @@ export class RouteConfigLoadEnd {
* - {@link NavigationError},
* - {@link RoutesRecognized},
* - {@link RouteConfigLoadStart},
* - {@link RouteConfigLoadEnd}

This comment has been minimized.

Copy link
@vicb

vicb Jun 22, 2017

Contributor

What about adding a timeline for the events ?

I think the docs could be improved here.

This comment has been minimized.

Copy link
@jasonaden

jasonaden Jun 22, 2017

Author Contributor

Agreed on the docs. I think there's a lot to do on that, and I want to get that done ASAP. Just not as part of this PR. I've started mapping this out for myself so I could actually write a good doc on it.

return of (p);
}
});
if (p.shouldActivate && preActivation.isActivating()) {

This comment has been minimized.

Copy link
@vicb

vicb Jun 22, 2017

Contributor

what is && preActivation.isActivating() ?

This comment has been minimized.

Copy link
@jasonaden

jasonaden Jun 22, 2017

Author Contributor

Basically I want to refactor the PreActivation class but can't do that at this time.

PreActivation determines if the route is going to activate by a combination of things. One is the shouldActivate, and the other is whether or not the canActivateChecks array has length.

const checks$ = from(this.canActivateChecks);
const runningChecks$ =
concatMap.call(checks$, (check: CanActivate) => this.runResolve(check.route));
return reduce.call(runningChecks$, (_: any, __: any) => _);
}

isDeactivating(): boolean { return this.canDeactivateChecks.length !== 0; }

This comment has been minimized.

Copy link
@vicb

vicb Jun 22, 2017

Contributor

hasDeactivateChecks ?

This comment has been minimized.

Copy link
@jasonaden

jasonaden Jun 22, 2017

Author Contributor

Same as above.

@vicb
Copy link
Contributor

left a comment

Changes LGTM.
Docs for event is sparse. Please take this opportunity to add more docs.

@jasonaden jasonaden force-pushed the jasonaden:feat_add_router_events branch from 8fbf1d3 to 2ca0086 Jun 23, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 23, 2017

The angular.io preview for 2ca0086 is available here.

@jasonaden jasonaden force-pushed the jasonaden:feat_add_router_events branch from 2ca0086 to 3a23f0e Jun 23, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 23, 2017

The angular.io preview for 3a23f0e is available here.

@vicb

vicb approved these changes Jun 26, 2017

@jasonaden jasonaden force-pushed the jasonaden:feat_add_router_events branch from 3a23f0e to 5e399ea Jun 27, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Jun 27, 2017

The angular.io preview for 5e399ea is available here.

@jasonaden jasonaden force-pushed the jasonaden:feat_add_router_events branch from 5e399ea to 683f880 Jun 27, 2017

@mary-poppins

This comment has been minimized.

@jasonaden jasonaden merged commit 8a1a989 into angular:master Jul 1, 2017

3 checks passed

cla/google All necessary CLAs are signed
code-review/pullapprove Approved by all reviewer groups.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.