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(dynamic overlay): prevent multiple onStable subscriptions #2494

Merged
merged 6 commits into from
Aug 31, 2020
Merged

fix(dynamic overlay): prevent multiple onStable subscriptions #2494

merged 6 commits into from
Aug 31, 2020

Conversation

yggg
Copy link
Contributor

@yggg yggg commented Aug 31, 2020

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

The main fix is 419ca7c. After the change, dynamic overlay unsubscribes from zone.onStable once overlay got destroyed. Before, every time the show method was called, we were creating an additional subscription, which leads to multiple updatePosition calls.

Changes from 3379373 is an additional fix, which ensures dynamic overlay context and config properties updating only once some property actually changed. Before, because of we were passing a new object to overlayConfig and context methods overlay was recreated on every ngOnChanges run. See commit description for details. UPD: 1763905 is a refactor of the way overlay config objects updated. First I decided to use change object from OnChanges hook, but we have tests which check if direct manipulation of component instance properties (not in the template) is also picked up. OnChanges hook doesn't track such changes, so I moved overlay config objects update code to the setters. This way you can change properties from anywhere (from template or component instance properties) and it'll work anyway.

The third commit 1840a9d, has an additional small fix. We need to call updatePosition once overlay content change, as it could affect overlay dimensions. It worked fine before because of the bug fixed in this PR.

Otherwise, every time the `show` method is called, we create an additional
subscription. It leads to multiple `updatePosition` calls.
Dynamic overlay handler compares context and configs objects by reference.
So when we passing a new object to `context` and `overlayConfig` methods
dynamic overlay handler threats them as a new objects and updates these
properties on the dynamic overlay. Changes on dynamic overlay context and
config causes disposal of existing overlay and creation of a new one.
This commit changes ensures config and context objects update only once
at least one of it's properties got updated. This prevent unnecessary overlay
disposal described above.
@yggg yggg changed the title fix(dynamic overlay): prevent multiple onStable subscriptions. fix(dynamic overlay): prevent multiple onStable subscriptions Aug 31, 2020
As content affects overlay dimensions.
Instead of using `ngOnChanges` as it won't be called if some
properties have been changed on component instances directly
(not in the template).
@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #2494 into master will increase coverage by 0.06%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #2494      +/-   ##
==========================================
+ Coverage   80.41%   80.48%   +0.06%     
==========================================
  Files         244      244              
  Lines        7180     7224      +44     
  Branches      787      792       +5     
==========================================
+ Hits         5774     5814      +40     
- Misses       1171     1172       +1     
- Partials      235      238       +3     
Impacted Files Coverage Δ
.../components/context-menu/context-menu.directive.ts 90.32% <90.00%> (+0.07%) ⬆️
.../components/cdk/overlay/dynamic/dynamic-overlay.ts 97.47% <100.00%> (-1.60%) ⬇️
...work/theme/components/popover/popover.directive.ts 93.87% <100.00%> (+0.85%) ⬆️
...work/theme/components/tooltip/tooltip.directive.ts 94.33% <100.00%> (+0.72%) ⬆️

@yggg yggg merged commit f22e87d into akveo:master Aug 31, 2020
@yggg yggg deleted the fix/dynamic-overlay-subscriptions-leak branch August 31, 2020 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant