Skip to content

feat(observers): allow for mutation observer options to be customized in content observer #13842

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crisbeto
Copy link
Member

Currently we always use the same set of MutationObserver options to monitor an element, however if the consumer knows what they want to monitor, they could gain a performance benefit from configuring the observer differently. These changes rework the ContentObserver to allow it to handle multiple observers per element that are cached based on their options.

@crisbeto crisbeto added the target: minor This PR is targeted for the next minor release label Oct 27, 2018
@crisbeto crisbeto requested a review from jelbourn as a code owner October 27, 2018 14:19
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 27, 2018
@crisbeto crisbeto force-pushed the content-observer-options branch 2 times, most recently from 2832d6f to a70216f Compare October 27, 2018 16:14
@jelbourn
Copy link
Member

jelbourn commented Nov 6, 2018

I worry this makes the cdkContentObserver too broad- it was intended to provide an easy/foolproof way of knowing when the content of an element changed.

What do you think about making a separate cdkMutationObserver that exposes all of the knobs for the underlying MutationObserver for people that want more fine-grained control?

@crisbeto
Copy link
Member Author

crisbeto commented Nov 8, 2018

The default consumption is as simple as before, the only difference is that people who know what they're doing can pass in some extra options. My concern is that if added another API, we'd have pretty much the same logic, but with the ability to pass in a settings object.

@jelbourn
Copy link
Member

jelbourn commented Dec 4, 2018

Coming back to this PR- was this requested somewhere? My main thought now is wanting to avoid the extra code if nobody has really asked for it.

@crisbeto
Copy link
Member Author

crisbeto commented Dec 5, 2018

It wasn't requested anywhere, but it's something I noticed when fixing issues that involved cdkObserveContent directive. For most of the cases, we're using the MutationObserver only to look for text changes, however we end up observing a bit more. There's also the attributes option which could be useful in some cases, but we don't expose to the user.

@ngbot
Copy link

ngbot bot commented Dec 19, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

2 similar comments
@ngbot
Copy link

ngbot bot commented Dec 19, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@ngbot
Copy link

ngbot bot commented Dec 19, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto crisbeto force-pushed the content-observer-options branch from a70216f to 97be7e0 Compare January 6, 2019 20:26
@crisbeto crisbeto force-pushed the content-observer-options branch from 97be7e0 to d9f5205 Compare January 20, 2019 14:07
@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@johanchouquet
Copy link
Contributor

Hi there, my understanding is that we would be able to observe if a specifig img tag for example would be loaded. If that's the case, I do feel like we developers have a true need of this, as I can read quite a lot of things about lazy loading images. And this is just 1 use case.

@mmalerba mmalerba removed their assignment Aug 12, 2019
@natsch93
Copy link

Any update on this, please? I would also like this since the content observer is unable to detect img src changes with the current configuration. Thank you.

… in content observer

Currently we always use the same set of `MutationObserver` options to monitor an element, however if the consumer knows what they want to monitor, they could gain a performance benefit from configuring the observer differently. These changes rework the `ContentObserver` to allow it to handle multiple observers per element that are cached based on their options.
@crisbeto crisbeto force-pushed the content-observer-options branch from d9f5205 to 7a90374 Compare November 24, 2019 11:53
@crisbeto
Copy link
Member Author

@jelbourn should we revisit this now that we've had a few requests for it?

@jelbourn
Copy link
Member

@crisbeto let's discuss in the next team meeting. I'm still of the opinion that, if we want to broaden the scope of this directive, at the very least the name needs to change.

@jelbourn
Copy link
Member

jelbourn commented Dec 5, 2019

We talked about this at our team meeting. Resolution:

  • Rename ContentObserver -> MutationObserverRegistry
  • Refactor CdkObserveContent -> CdkObserveMutations with full options
  • cdkObserveContent remains for textContent only

@mcalmus
Copy link
Contributor

mcalmus commented May 21, 2020

Any chance we can get this in v10? I'd really like to use it to swap out images based upon some attributes.

@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@josephperrott josephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022
@JWess
Copy link

JWess commented Aug 2, 2023

I was hoping to use this to observe an HTML attribute, but alas, I guess I'll be using native MutationObserver instead... 😞

@josephperrott josephperrott requested review from a team as code owners December 18, 2024 17:40
@josephperrott josephperrott requested review from amysorto and removed request for a team December 18, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.