feat(router): Added public access to current instruction

[brandonroberts]

This provides public access to the current instruction and exposes the root router on each child router.

cc: @btford @petebacondarwin

[petebacondarwin]

This is a reasonable idea. What do you think @btford ?

[brandonroberts]

Finally got a green build on this one

[petebacondarwin]

I would like to think through the various use cases for accessing the complete instruction/URL/route information before agreeing to merge this...

[brandonroberts]

One use case is building a breadcrumb component. There isn't an easier way to get the instruction tree from a top down approach without accessing private properties. One way to get the full instruction is to subscribe to the router, get the url and resolve router.recognize promise to turn that url into the instruction.

[brandonroberts]

Another use case is reloading the current route programmatically. If you try to use router.renavigate, it doesn't work unless you used router.navigateByUrl previously. If that's the intended behavior, you could use router.navigateByInstruction(router.currentInstruction()) to reload the current route.

[petebacondarwin]

I don't believe that we need this change for the use cases defined in #5335

But I accept @brandonroberts' first use case for building a breadcrumb tree. But I wonder if we need to make an API distinction between the "relative" current instruction, which is relative to the current router, and the "absolute" current instruction, which is relative to the root router?

In the second use case, I believe that the renavigate() method is supposed to be used when the route configuration may have changed and that the URL may no longer correspond to the current instruction. So I don't think that you would use it for arbitrary reloading of routes.

I am not sure what the real world use case for reloading the current instruction is... Can you give one?

[brandonroberts]

@petebacondarwin I think a relative current instruction could be useful. My issue is that the router isn't going to force reload its parent routes and unless told not to, it will use reuse them. We already have the current instruction that are provided with the lifecycle hooks. Granted you can't use those for navigation and you can't use them to walk back up the route tree, but they are there.

One use case would be to force reload the entire ui for the current route after logging in through a modal. A user goes to a page that is publicly accessible but only shows limited information until the user logs in. The user triggers an action to login, a modal is displayed, they login and the entire route which is composed of parent/child routes is refreshed.

Maybe there needs to be a way force a reload instead?

[petebacondarwin]

So what you are saying is that the resolution of a particular URL/route-instruction could be dependent upon information that is not contained in the URL/route-instruction such as whether the user is logged in.

[brandonroberts]

Yes

[petebacondarwin]

OK, so there are two goals:

1. support breadcrumb generation.
2. support forcing a reload of the current instruction.

The first is fixed by this PR - by accessing the top level instruction, which would allow the developer to traverse down the instructions - but I think we should call the method something slightly different to emphasize that it is getting the complete instruction from the root. I was considering if we should expose the rootRouter directly on each router, then just expose the currentInstruction publicly. To get the full instruction we could then do router.root.currentInstruction. What do you think @brandonroberts ?

The second is not even fixable at the moment by this PR, since calling navigateByInstruction will just reuse the current components unless they explicitly prevent reuse by implementing $canReuse and so they may not automatically update their state... Again what do you think @brandonroberts ?

[brandonroberts]

With the first one, I'm fine with calling the method something different and exposing the current instruction. I think what you have proposed would be easier than traversing up the router.parent to get to the root. Would you consider making the RootRouter injectable in route components like you did with ng1 component router? I don't know if that would be confusing to be able to inject the components router and the root router, but they serve different purposes.

Agreed on the second issue as well.

[brandonroberts]

@petebacondarwin I updated this PR to include the changes we discussed to access the root router and make the current instruction public.

[MonkeyDo]

Thanks @brandonroberts, that looks nice and easy.

[petebacondarwin]

I'll take a look later today!!

[petebacondarwin]

I don't think this has to be generated here, right?
Can't we just have a straight attribute which is assigned in the constructor?

class RootRouter {
  constructor() {
    this.root = this;
  }
}

class ChildRouter {
  constructor(parent) {
    this.root = parent.root;
  }
}

[petebacondarwin]

One small note but I think the general idea is good.
I think we would need @btford or another Googler to chime in before we go ahead and modify the public API.

[brandonroberts]

@petebacondarwin good idea. I made the change you suggested.

[petebacondarwin]

Sorry to be a pain but we can make providing the root even more explicit by including it as a constructor parameter here:

constructor(public registry: RouteRegistry, public parent: Router, public root: Router, public hostComponent: any) {}

Then we can ditch the declaration further up.

[brandonroberts]

No problem. I moved the root property to the end of the constructor and made it optional to satisfy TypeScript. The ROUTER_PROVIDERS provides the root router, and it won't have a root.

  constructor(public registry: RouteRegistry, public parent: Router, public hostComponent: any,
              public root?: Router) {}

Is that ok?

[petebacondarwin]

Great. Let's get @IgorMinar or @mhevery to review the API change.

[mhevery]

API change is fine.

[brandonroberts]

Cool. I got an error on the build but it wasn't caused by my changes.

[petebacondarwin]

It was a flake! I reran the build and it is green now.

[brandonroberts]

Thanks!

[vikerman]

@petebacondarwin - Is this PR LGTM to be merged? FYI - We look for both pr_state:LGTM and pr_action: merge to look for PRs to merge.

[vikerman]

TAP Green

[vikerman]

Manually merged to master.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}