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

fix(router): RouterLinkActive should update its state right after che… #19449

Closed

Conversation

vsavkin
Copy link
Contributor

@vsavkin vsavkin commented Sep 27, 2017

…cking the children

Closes #18983

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] 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:

What is the current behavior?

Issue Number:18983

What is the new behavior?

In general, directives depending on their children break the change detection data flow, unless they do it only to update the DOM. This is what RouterLinkActive used to do--it only updated the DOM. Adding exportAs to it made it problematic. Now this directive's state depends on the children, and the state is exposed to other directives. This is why there are so many issues with it.

I fixed the directive the same we do it everywhere else: I moved the behavior that breaks the data flow into a promise (we already had a promise before--but it wasn't working properly. we should never update the state after we update the dom. The dom should reflect the state).

There are other ways to solve this problem without breaking the change detection data flow (e.g., defining a separate pipe for that), but they are breaking.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@jasonaden jasonaden added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Oct 17, 2017
@tbosch tbosch added this to the Merge-queue milestone Oct 17, 2017
@tbosch tbosch closed this in c569b75 Oct 18, 2017
tbosch added a commit to tbosch/angular that referenced this pull request Oct 18, 2017
tbosch added a commit that referenced this pull request Oct 18, 2017
…fter checking the children (#19449)"

This reverts commit c569b75.
As it was synched together with 5a9ed2d
which broke an internal test.
@tbosch tbosch reopened this Oct 18, 2017
@tbosch
Copy link
Contributor

tbosch commented Oct 18, 2017

Note: This looked good on a global presubmit, but was reverted because it was synced with another change that had to be reverted.

@tbosch tbosch closed this in 6f2939d Oct 18, 2017
@cexbrayat
Copy link
Member

@vsavkin @tbosch Looks like this change breaks unit tests, as this adds the necessity of an extra tick(), like Victor did in integration.spec.ts. Maybe it's worth mentioning in the changelog?

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018
jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018
…fter checking the children (angular#19449)"

This reverts commit c569b75.
As it was synched together with 5a9ed2d
which broke an internal test.
jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018
@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 Sep 12, 2019
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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RouterLinkActive race condition with RouterLink created dynamically from template ref
5 participants