Skip to content

/ angular

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

Jump to bottom

fix(router): add missing outlet events to RouterOutletContract #42431

Closed
wants to merge 1 commit into from
Closed

fix(router): add missing outlet events to RouterOutletContract #42431

wants to merge 1 commit into from
+16 −7

Conversation

@smasala
Copy link
Contributor

@smasala smasala commented May 31, 2021

PR adds activateEvents and deactivateEvents to the RouterOutletContract.

This is needed in our case where we dynamically render components via the Module Federation at some point in the future. Without this the active component is always ɵEmptyOutletComponent and not the actual component which is eventually created by an external route, hence why these events are needed so that we can capture all components rendered at any given time.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

Simply updating the public api interface which now causes type errors in angular 12 but is technically still available.

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information
@google-cla google-cla bot added the cla: yes label May 31, 2021
@pullapprove pullapprove bot requested a review from atscott May 31, 2021
@smasala smasala force-pushed the smasala:patch-1 branch 3 times, most recently from aa36b67 to f645559 May 31, 2021
@ngbot ngbot bot added this to the Backlog milestone Jun 3, 2021
@atscott
Copy link
Contributor

@atscott atscott commented Jul 23, 2021

These events aren't part of the contract between the Router and the outlet. The Router doesn't listen to these events so they're not needed in the interface.

@atscott atscott closed this Jul 23, 2021
@smasala
Copy link
Contributor Author

@smasala smasala commented Jul 23, 2021

@atscott If the router doesn't listen to them, how am I supposed to access them in a typed manner as they are triggered?

@atscott
Copy link
Contributor

@atscott atscott commented Jul 23, 2021 •
edited

@smasala Can you please file an issue report and outline your use-case? Are you reading from the OutletContext/ChildrenOutletContext?

Also, this change would be breaking because the new function on the interface would need to be implemented by the classes.

@atscott atscott reopened this Jul 23, 2021
@atscott atscott modified the milestones: Backlog, v13-candidates Jul 23, 2021
@smasala smasala mentioned this pull request Jul 26, 2021
Open
@smasala
Copy link
Contributor Author

@smasala smasala commented Jul 26, 2021

@atscott #42959 this ok?

@smasala
Copy link
Contributor Author

@smasala smasala commented Jul 26, 2021

@atscott this isn't breaking as the events are there, just not exposed on the interface. I'm still able to access the events as in v11, just in v12 I need to cast the object so that I can get them without a TS error.

@alfaproject
Copy link
Contributor

@alfaproject alfaproject commented Jul 26, 2021

That’s exactly why it’s a breaking change. You have to change something when updating (;

@smasala
Copy link
Contributor Author

@smasala smasala commented Jul 26, 2021

@alfaproject breaking is a matter of definition. If this PR is merged, my code works as before with no further changes required (not breaking) and any migrations from v11 to this merged PR would also continue to work. The only difference for me or current Ng 12 applications would be that any casting to the RouterOutlet would be redundant if this PR were merged.

@atscott
Copy link
Contributor

@atscott atscott commented Jul 26, 2021

@smasala The way the public API works would still make this a breaking change. Any new implementers of the interface in v12 would break with this update.

We can make this non-breaking by having the events be optional and we can merge it for the 12.2.0 release next week. If you could update the PR to define them as activateEvents?: EventEmitter<any>; and deactivateEvents?: EventEmitter<any>; and regenerate the public API golden, we can work on getting this merged ASAP.

@smasala smasala force-pushed the smasala:patch-1 branch from f645559 to 0b2898c Jul 26, 2021
@smasala
Copy link
Contributor Author

@smasala smasala commented Jul 26, 2021 •
edited

@atscott updated. Thanks btw 👍 😊

@atscott atscott force-pushed the smasala:patch-1 branch from 0b2898c to b216840 Jul 26, 2021
@atscott
atscott approved these changes Jul 26, 2021
View changes
Copy link
Contributor

@atscott atscott left a comment

reviewed-for: fw-router
reviewed-for: public-api
@atscott atscott linked an issue that may be closed by this pull request Jul 26, 2021
Expose activateEvents and deactivateEvents via RouterOutletContract #42959
Open
@AndrewKushnir
AndrewKushnir reviewed Jul 26, 2021
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

I think we could replace any with unknown in this change (to avoid extra anys in public API surface). The RouterOutletContract mimics the RouterOutlet class structure, but there are cases already (see attach and detach) where RouterOutletContract has unknowns and the RouterOutlet class that implements it has any.

@atscott what do you think about using unknown in this change?
@pullapprove pullapprove bot requested a review from IgorMinar Jul 26, 2021
@smasala @atscott
fix(router): add missing outlet events to RouterOutletContract
4aa9fb5 
Exposes both activateEvents and deactivateEvents as the original outlet interface did.
@atscott atscott force-pushed the smasala:patch-1 branch from b216840 to 4aa9fb5 Jul 26, 2021
@atscott
Copy link
Contributor

@atscott atscott commented Jul 26, 2021

@AndrewKushnir That's a fair point and a good idea. I went ahead and made that change.

presubmit

@AndrewKushnir
AndrewKushnir approved these changes Jul 26, 2021
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Thanks @smasala and @atscott for making this change! 👍

Reviewed-for: public-api
@jessicajaniuk
jessicajaniuk approved these changes Jul 27, 2021
View changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

LGTM 🍪

reviewed-for: public-api
@atscott atscott removed request for IgorMinar, jelbourn and zarend Jul 27, 2021
@dylhunn dylhunn closed this in 0d81b00 Jul 27, 2021
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Aug 27, 2021

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 Aug 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott

@AndrewKushnir AndrewKushnir

@jessicajaniuk jessicajaniuk

Assignees
No one assigned
Labels
action: merge cla: yes comp: router feature regression target: minor
Projects
None yet
Milestone
v13-candidates
Linked issues

Successfully merging this pull request may close these issues.

Expose activateEvents and deactivateEvents via RouterOutletContract
5 participants
@smasala @atscott @alfaproject @AndrewKushnir @jessicajaniuk