perf(progress-circle): clean up animation on destroy #617
Conversation
googlebot
added
the
cla: yes
|
@crisbeto Can you add a unit test for this? |
crisbeto
force-pushed
the
perf/circle-cleanup
branch
2 times, most recently
from
992822e
to
c008804
Compare
|
Done @jelbourn, although some of the CI builds are failing because of unrelated issues. (missing |
| .then((fixture) => { | ||
| fixture.detectChanges(); | ||
| let progressElement = getChildDebugElement(fixture.debugElement, 'md-progress-circle'); | ||
| expect(progressElement.componentInstance._interdeterminateInterval).toBeTruthy(); |
There was a problem hiding this comment.
You can't access the private member here; The field either needs to be changed to /** @internal */ or we need some other proxy for checking that the interval has stopped.
|
Only remaining CI errors are lint |
crisbeto
force-pushed
the
perf/circle-cleanup
branch
2 times, most recently
from
c7a6abf
to
bedbfda
Compare
|
It's updated to use a getter/setter on the interval and resetting it on the setter instead to avoid intervals stacking up. The CI is still throwing a Node ELIFECYCLE error, though. |
|
That node error message is just garbage that gets thrown when there's a tslint error. The actual problem is just above: |
| /** The id of the last requested animation. */ | ||
| private _lastAnimationId: number = 0; | ||
|
|
||
| /** The id of the indeterminate interval. */ | ||
| private _interdeterminateInterval: number; | ||
| get interdeterminateInterval() { |
There was a problem hiding this comment.
This should be marked as /** @internal */
crisbeto
force-pushed
the
perf/circle-cleanup
branch
from
bedbfda
to
146b497
Compare
|
There we go. @jelbourn. EDIT: Regarding wrapping the tests in |
| /** The id of the indeterminate interval. */ | ||
| /** | ||
| * The id of the indeterminate interval. | ||
| * @internal |
There was a problem hiding this comment.
The @internal actually goes on the public member rather than the private one; this tells TypeScript to omit the property when outputting a .d.ts file so that downstream users can't depend on it.
There was a problem hiding this comment.
Sorry, I'm still not too familiar with all the ng2 concepts. It's updated now.
The indeterminate animation is based on an interval that keeps running, even after the element has been destroyed. This change cleans up when the element is destroyed. Also adds an extra null check for `performance.now`.
crisbeto
force-pushed
the
perf/circle-cleanup
branch
from
146b497
to
2ebbeb3
Compare
|
LGTM |
|
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. |
The indeterminate animation is based on an interval that keeps running, even after the element has been destroyed.
This change cleans up when the element is destroyed.
Also adds an extra null check for
performance.now.