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): emit activate/deactivate events when an outlet gets attached/detached #43333

Conversation

dimakuba
Copy link
Contributor

@dimakuba dimakuba commented Sep 3, 2021

Previously there was no way to subscribe for attach/detach events from the RouterOutlet when an outlet got attached/detached with RouteReuseStrategy. The changes add new outputs attached/detached for the RouterOutlet to emit events when an outlet gets attached/detached.

@google-cla google-cla bot added the cla: yes label Sep 3, 2021
@pullapprove pullapprove bot requested a review from atscott September 3, 2021 14:36
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

This changes the meaning of the activate and deactivate events. The attach and detach are similar but not exactly the same. Especially when comparing detach with deactivate, the deactivate emits after the component is destroyed.

In fact, this behavior is specifically documented by the activate and deactivate events.

A router outlet emits an activate event when a new component is instantiated,
and a deactivate event when a component is destroyed.

This change is contrary to the documented behavior. Please open a feature request for new events attached and detached rather than changing the meaning of the current activate and deactivate events.

@atscott atscott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: router target: minor This PR is targeted for the next minor release labels Sep 3, 2021
@ngbot ngbot bot modified the milestone: Backlog Sep 3, 2021
@atscott atscott added router: directives RouterLink, RouterLinkActive, RouterOutlet etc. feature Issue that requests a new feature freq1: low labels Sep 3, 2021
@dimakuba
Copy link
Contributor Author

dimakuba commented Sep 9, 2021

This changes the meaning of the activate and deactivate events. The attach and detach are similar but not exactly the same. Especially when comparing detach with deactivate, the deactivate emits after the component is destroyed.

In fact, this behavior is specifically documented by the activate and deactivate events.

A router outlet emits an activate event when a new component is instantiated,
and a deactivate event when a component is destroyed.

This change is contrary to the documented behavior. Please open a feature request for new events attached and detached rather than changing the meaning of the current activate and deactivate events.

Thanks! Good point, by creating new outputs we avoid also potential breaking changes.

@dimakuba dimakuba force-pushed the emit-event-when-outlet-gets-attached-detached branch 2 times, most recently from 514990f to ca8a4dd Compare September 9, 2021 15:57
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

Just a note that it would be nice to mention a use-case for this feature either in the commit message or a resolved issue.

Edit: For example, another option in #43251 would be to use the outlet context's onAttached event rather than the Router's events

  onNavBackToCachedComponent(outletName: string) {
    const context = this.contexts.getContext(outletName);
    context.outlet.attachedEvents.pipe(take(1)).subscribe(() => {
      // Do something
    });

packages/router/src/directives/router_outlet.ts Outdated Show resolved Hide resolved
packages/router/src/directives/router_outlet.ts Outdated Show resolved Hide resolved
@dimakuba dimakuba force-pushed the emit-event-when-outlet-gets-attached-detached branch from ca8a4dd to a81a690 Compare September 10, 2021 14:42
packages/router/src/directives/router_outlet.ts Outdated Show resolved Hide resolved
packages/router/src/directives/router_outlet.ts Outdated Show resolved Hide resolved
@dimakuba dimakuba force-pushed the emit-event-when-outlet-gets-attached-detached branch from a81a690 to 3851e42 Compare September 13, 2021 07:23
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@atscott
Copy link
Contributor

atscott commented Sep 13, 2021

presubmit

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 jelbourn September 14, 2021 23:05
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM for api changes! Thank you, @dimakuba. This is a nice addition.

Reviewed-for: public-api

@atscott atscott removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 24, 2021
…ached/detached

Previously the events of `RouterOutlet` (activate/deactivate) were not fired
when an outlet got attached/detached with `RouteReuseStrategy`. The changes configure
`RouterOutlet` to emit events when an outlet gets attached/detached.

Fixes angular#25521, angular#20501
@dimakuba dimakuba force-pushed the emit-event-when-outlet-gets-attached-detached branch from 3851e42 to 7cbbf26 Compare September 27, 2021 07:41
@atscott atscott added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Sep 27, 2021
@alxhub alxhub closed this in 4f3beff Sep 28, 2021
@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 Oct 30, 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 area: router cla: yes feature Issue that requests a new feature freq1: low merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note router: directives RouterLink, RouterLinkActive, RouterOutlet etc. 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

5 participants