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): Allow for custom router outlet implementations #40827

Closed
wants to merge 2 commits into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Feb 12, 2021

This PR formalizes, documents, and makes public the router outlet contract.

The set of RouterOutlet methods used by the Router has not changed
in over 4 years, since the introduction of route reuse strategies.

Creation of custom router outlets is already possible and is used by the
Ionic framework (https://github.com/ionic-team/ionic-framework/blob/master/angular/src/directives/navigation/ion-router-outlet.ts).
There is a small "hack" that is needed to make this work, which is that
outlets must register with ChildrenOutletContexts, but it currently
only accepts our RouterOutlet.

By exposing the interface the Router uses to activate and deactivate
routes through outlets, we allow for developers to more easily and safely
extend the Router and have fine-tuned control over navigation and component
activation that fits project requirements.

// cc @liamdebeasi @mhartington

@atscott atscott added action: review The PR is still awaiting reviews from at least one requested reviewer area: router target: minor This PR is targeted for the next minor release labels Feb 12, 2021
@ngbot ngbot bot modified the milestone: Backlog Feb 12, 2021
@google-cla google-cla bot added the cla: yes label Feb 12, 2021
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @atscott 👍

Reviewed-for: public-api

@mary-poppins
Copy link

You can preview 98d6293 at https://pr40827-98d6293.ngbuilds.io/.
You can preview bd118ed at https://pr40827-bd118ed.ngbuilds.io/.

@mary-poppins
Copy link

You can preview aeaae1e at https://pr40827-aeaae1e.ngbuilds.io/.

This PR formalizes, documents, and makes public the router outlet contract.

The set of `RouterOutlet` methods used by the `Router` has not changed
in over 4 years, since the introduction of route reuse strategies.

Creation of custom router outlets is already possible and is used by the
Ionic framework
(https://github.com/ionic-team/ionic-framework/blob/master/angular/src/directives/navigation/ion-router-outlet.ts).
There is a small "hack" that is needed to make this work, which is that
outlets must register with `ChildrenOutletContexts`, but it currently
only accepts our `RouterOutlet`.

By exposing the interface the `Router` uses to activate and deactivate
routes through outlets, we allow for developers to more easily and safely
extend the `Router` and have fine-tuned control over navigation and component
activation that fits project requirements.
@mary-poppins
Copy link

You can preview 6d2d33c at https://pr40827-6d2d33c.ngbuilds.io/.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Should creating a custom router-outlet also be documented in the router guide?

goldens/public-api/router/router.d.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested a review from jelbourn February 13, 2021 00:24
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

LGTM! 🍪

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from IgorMinar February 13, 2021 01:22
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Ideally I would have expected the interface to be RouterOutlet and then our implementation to be something like DefaultRouterOutlet. I assume that changing the name of this class would be a breaking change?

Adding Contract to the end appears to be a workaround to avoid such a breaking change, but it feels like a new naming convention that we have not used in any of the rest of the framework, and so a bit out of place or awkward.

Could we consider how bad the breaking change would be to change the class name?

@atscott
Copy link
Contributor Author

atscott commented Feb 16, 2021

Ideally I would have expected the interface to be RouterOutlet and then our implementation to be something like DefaultRouterOutlet. I assume that changing the name of this class would be a breaking change?

Adding Contract to the end appears to be a workaround to avoid such a breaking change, but it feels like a new naming convention that we have not used in any of the rest of the framework, and so a bit out of place or awkward.

Could we consider how bad the breaking change would be to change the class name?

@petebacondarwin Yea, this was done to avoid the breaking change. I am open to bikeshedding the name of the interface but strongly feel that changing the class name would be a mistake even though the migration wouldn't necessarily be difficult. My two bullet points on this are:

  • Rolling your own router outlet is an extremely rare case. This change should really only be a formality for those who really want to do it but should not affect the 99.9% of users who don't need it.
  • Changing the name of the outlet has other ramifications as well: documentation, training materials, the selector, and the fact that it's had this name since the beginning of time.

If we decide that there's no good interface name, I'd be more inclined to scrap this PR entirely. What about NgRouterOutlet, AngularRouterOutlet, IRouterOutlet, or changing to an abstract class and naming it BaseRouterOutlet?

Should creating a custom router-outlet also be documented in the router guide?

@jelbourn I don't feel that this is necessary at the moment. If it seems like something more devs are beginning to use, we could add something. For now, I think it's more of something that would be discovered organically from absolute necessity of needing to modify the built-in outlet behavior.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Oh if only we could have called it RouterOutletDirective 🥰

I wonder how many applications actually refer to the class itself? I guess it could be injected into child components/directices?

If we can't change the name of the class then I'm happy to go with ...Contract as the interface. We could bikeshed it but I don't think there is likely to be a better name.

Reviewed-for: public-api

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Feb 16, 2021

What about NgRouterOutlet, AngularRouterOutlet, IRouterOutlet, or changing to an abstract class and naming it BaseRouterOutlet?

That's a great question. I believe we could use this PR as an example and try to come up with (and document?) the naming convention/recommendation for the interfaces/abstract classes/regular classes that we use in public API (so that we are consistent). As a part of this discussion we can look at existing patterns in the framework as well (to see if there are some similar cases). Since this PR doesn't target "patch" (so it'd be a part of v12), this should not delay the changes. @petebacondarwin @atscott what do you think?

@petebacondarwin
Copy link
Member

@AndrewKushnir - I feel that the boat has sailed for this case, but it would be good to discuss naming conventions going forward. Or at least discuss the potential difficulties that could come up when trying to future proof choices of names.

@atscott
Copy link
Contributor Author

atscott commented Feb 16, 2021

I wonder how many applications actually refer to the class itself? I guess it could be injected into child components/directices?

Not many: only about 60 in all of g3. Of these, I think the most common use-case is @ViewChild(RouterOutlet). Some are injecting in child classes, and some others are using it for querying in tests.

@mary-poppins
Copy link

You can preview 03c7c23 at https://pr40827-03c7c23.ngbuilds.io/.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM; I agree that the Contract part of the name is a bit unfortunate for its inconsistency, but this is a niche enough API that I don't think it's a huge deal

Reviewed-for: public-api

@atscott atscott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 19, 2021
@atscott
Copy link
Contributor Author

atscott commented Feb 19, 2021

presubmit

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview area: router cla: yes target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants