-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
feat(observe-content): add debounce option and other improvements #2404
feat(observe-content): add debounce option and other improvements #2404
Conversation
5db36e9
to
3351381
Compare
95f052a
to
f91d2e8
Compare
@jelbourn I added the MutationObserver to the providers in order to be able to stub it in the unit tests. I'm not sure whether this is the best way to do it, though. In particular, this line felt a little weird, but TS was complaining if I tried to inject the usual way and use it as a constructor. Something like |
f91d2e8
to
57a894f
Compare
src/lib/core/util/debounce.ts
Outdated
* @param delay Amount of milliseconds to wait before calling the function. | ||
* @param context Context in which to call the function. | ||
*/ | ||
export function debounce(func: Function, delay: number, context?: any): Function { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't rxjs have a debounce operator already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while since I submitted this PR, but IIRC we can't really use it here because the callback is exposed through an EventEmitter which is consumed through the template; <div (cdkObserveContent)="someCallback()">
. Technically we could debounce it with Rxjs inside the consumer, but that would make it really inconvenient (we'd need another observable that just proxies this one).
Another alternative for debouncing it through Rxjs could be to have an exportAs
for the ObserveContent directive and using a ViewChild to access the EventEmitter directly, but that might look weird as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to do something like
private _event = new EventEmitter<void>();
@Output('cdkObserveContent')
get event(): Observable<void> {
return this.debounce ?
this._event.asObservable().debounceTime(this.debounce) :
this._event.asObservable();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That didn't seem to work, but I managed to work around it. Can you take another look at some point?
78c8bc1
to
c5ba74c
Compare
@crisbeto looks like just some minor CI failures |
c5ba74c
to
9122ebe
Compare
@jelbourn can you take another look at this? The CI issues ended up being slightly more complicated, because of the |
422e579
to
eb2e454
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor comment
|
||
ngAfterContentInit() { | ||
this._observer = new MutationObserver(mutations => mutations.forEach(() => this.event.emit())); | ||
this._debouncer | ||
.debounceTime(this.debounce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we conditionally add debounceTime
to the chain if debounce !== 0
? Andrew noticed when working on the scroll dispatcher that auditTime
incurred a cost even when it was set to zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also rebased and fixed the fact the debouncer wasn't being completed.
* Adds a reusable utility for debouncing a function. * Adds the ability to debounce the changes from the `cdkObserveContent` directive. * Makes the `cdkObserveContent` directive pass back the `MutationRecord` to the `EventEmitter`. * Fires the callback once per mutation event, instead of once per `MutationRecord`. Relates to angular#2372.
eb2e454
to
19b936a
Compare
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. |
cdkObserveContent
directive.cdkObserveContent
directive pass back theMutationRecord
to theEventEmitter
.MutationRecord
.Relates to #2372.