fix(core): viewport trigger deregistering callbacks multiple times #52115
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crisbeto
added
action: review
crisbeto
commented
Oct 9, 2023
AndrewKushnir
approved these changes
Oct 9, 2023
JoostK
reviewed
Oct 9, 2023
| @@ -176,6 +176,11 @@ class DeferIntersectionManager { | |||
| entry.callbacks.add(callback); | |||
|
|
|||
| return () => { | |||
| // It's possible that a different cleanup callback fully removed this element already. | |||
| if (!this.viewportTriggers.has(trigger)) { | |||
There was a problem hiding this comment.
You could save some bytes using
if (!entry!.callbacks.delete(callback)) {
return;
}
There was a problem hiding this comment.
(I'd like to ask for a small comment in the code above the if in this case :))
AndrewKushnir
removed
the
action: review
Adds a check to the viewport cleanup function to prevent it from re-processing elements that have been fully cleaned up, because it can lead to the `IntersectionObserver` being destroyed even though there are still pending triggers. This can happen, because we have cleanup callbacks both for the block is loaded, but also when the placeholder view is destroyed. Fixes angular#52113.
crisbeto
force-pushed
the
52113/viewport-cleanup-multiple
branch
from
eac8cce
to
0efeb20
Compare
|
I found a way to capture it in a test. |
crisbeto
added
action: merge
|
Caretaker note: the CI is passing, but it's not green because the CI is referring to a check that doesn't exist. |
|
This PR was merged into the repository by commit d5dad3e. |
crisbeto
added a commit
to crisbeto/angular
that referenced
this pull request
Oct 11, 2023
When the `viewport` triggers were first introduced, we ended up having to use a service to keep track of them, because using the same global event handling as the other events led to some inconsistent test failures. It looks like the failures were caused by the same bug fixed angular#52115 so now we can switch back to the previous approach which is a bit more compact.
crisbeto
added a commit
to crisbeto/angular
that referenced
this pull request
Oct 11, 2023
When the `viewport` triggers were first introduced, we ended up having to use a service to keep track of them, because using the same global event handling as the other events led to some inconsistent test failures. It looks like the failures were caused by the same bug fixed angular#52115 so now we can switch back to the previous approach which is a bit more compact.
crisbeto
added a commit
to crisbeto/angular
that referenced
this pull request
Oct 12, 2023
When the `viewport` triggers were first introduced, we ended up having to use a service to keep track of them, because using the same global event handling as the other events led to some inconsistent test failures. It looks like the failures were caused by the same bug fixed angular#52115 so now we can switch back to the previous approach which is a bit more compact.
pkozlowski-opensource
pushed a commit
that referenced
this pull request
Oct 13, 2023
When the `viewport` triggers were first introduced, we ended up having to use a service to keep track of them, because using the same global event handling as the other events led to some inconsistent test failures. It looks like the failures were caused by the same bug fixed #52115 so now we can switch back to the previous approach which is a bit more compact. PR Close #52156
pkozlowski-opensource
pushed a commit
that referenced
this pull request
Oct 13, 2023
When the `viewport` triggers were first introduced, we ended up having to use a service to keep track of them, because using the same global event handling as the other events led to some inconsistent test failures. It looks like the failures were caused by the same bug fixed #52115 so now we can switch back to the previous approach which is a bit more compact. PR Close #52156
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
ChellappanRajan
pushed a commit
to ChellappanRajan/angular
that referenced
this pull request
Jan 23, 2024
…ngular#52115) Adds a check to the viewport cleanup function to prevent it from re-processing elements that have been fully cleaned up, because it can lead to the `IntersectionObserver` being destroyed even though there are still pending triggers. This can happen, because we have cleanup callbacks both for the block is loaded, but also when the placeholder view is destroyed. Fixes angular#52113. PR Close angular#52115
ChellappanRajan
pushed a commit
to ChellappanRajan/angular
that referenced
this pull request
Jan 23, 2024
When the `viewport` triggers were first introduced, we ended up having to use a service to keep track of them, because using the same global event handling as the other events led to some inconsistent test failures. It looks like the failures were caused by the same bug fixed angular#52115 so now we can switch back to the previous approach which is a bit more compact. PR Close angular#52156
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a check to the viewport cleanup function to prevent it from re-processing elements that have been fully cleaned up, because it can lead to the
IntersectionObserverbeing destroyed even though there are still pending triggers. This can happen, because we have cleanup callbacks both for the block is loaded, but also when the placeholder view is destroyed.Fixes #52113.