feat(observe-content): refactor so logic can be used without directive #11170

mmalerba · 2018-05-04T22:17:13Z

No description provided.

crisbeto · 2018-05-05T11:26:25Z

src/cdk/observers/observe-content.ts

+export class ContentObserverFactory {
+  constructor(private _mutationObserverFactory: MutationObserverFactory, private _ngZone: NgZone) {}
+
+  create(element: Element, debounce?: number) {


Shouldn't we call this one observe?

This doesn't actually start observing, it just creates the observer

crisbeto · 2018-05-05T11:34:38Z

src/cdk/observers/observe-content.ts

+
+
+/** A class that observes an element for content changes. */
+export class ContentObserver {


We could simplify the whole approach a bit by not having this class to begin with. Instead, ContentObserverFactory.observe can return an observable which creates a MutationObserver when something subscribes to it and then tears down the observer when there are no more subscriptions. We did something similar in the ScrollDispatcher. It means that we lose the ability to pause an observer and resume it with the same MutationObserver, but AFAIK there isn't much overhead to creating mutation observers.

I propose keeping ContentObserverFactory how it was before (docs private API that we use internally), and then making ContentObserver an injectable that works like ScrollDispatcher and BreakpointObserver; I think having the user need to interact with two different types is more work than it has to be

crisbeto · 2018-05-05T11:35:45Z

src/cdk/observers/observe-content.spec.ts

@@ -120,6 +120,57 @@ describe('Observe content', () => {
  });
 });

+describe('Observe content injectable', () => {


Since all the logic is in the injectable, maybe we should put all of the tests under this suite?

I wanted to leave the above tests alone to show that this doesn't change the behavior of the directive. Yes, we could add more tests here as well, but it felt like it would just be retesting the same logic

mmalerba · 2018-05-08T22:37:35Z

Comments addressed. I also removed the part where we observe outside the zone. I think if it's a public API that returns an Observable, users will probably expect that Observable to emit inside the zone. Does this make sense to you guys?

jelbourn · 2018-05-09T17:15:47Z

src/cdk/observers/observe-content.ts

+/** A factory that creates ContentObservers. */
+@Injectable({providedIn: 'root'})
+export class ContentObserver {
+  private _observedElements = new Map<Element, {


This could be a WeakMap

Whoops, I made it a Map so I could loop on destroy and clean up, but forgot to actually do that.

jelbourn · 2018-05-09T17:16:12Z

src/cdk/observers/observe-content.ts

+
+  constructor(private _mutationObserverFactory: MutationObserverFactory) {}
+
+  observe(element: Element, debounce?: number): Observable<MutationRecord[]> {


As user-facing JsDoc here and other method descriptions throughout?

jelbourn · 2018-05-09T17:18:24Z

src/cdk/observers/observe-content.ts

+
+  constructor(private _mutationObserverFactory: MutationObserverFactory) {}
+
+  observe(element: Element, debounce?: number): Observable<MutationRecord[]> {


What's the reasoning behind making debouce part of the API rather than letting subscribers debounce (or throttle or filter, etc) however they want?

I put it here because it was part of the directive's logic, but I agree it doesn't really belong here, will remove.

jelbourn · 2018-05-09T17:22:14Z

src/cdk/observers/observe-content.ts

+
+  private _unobserveElement(element: Element) {
+    if (this._observedElements.has(element)) {
+      if (!--this._observedElements.get(element)!.count) {


Could you break up the decrement and the conditional? It's generally easier to understand that way, since many people have a hard time remembering the different between --value and value--

jelbourn · 2018-05-09T17:26:28Z

src/cdk/observers/observe-content.spec.ts

+
+          subscription.unsubscribe();
+        })));
+  });


Should add a test for observing the same element twice, unobserving once and ensuring you still get events.

Should add a test for debouncing if it's part of the API

mmalerba · 2018-05-09T18:51:36Z

PTAL

jelbourn

LGTM

ngbot · 2018-05-09T21:20:47Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
status "continuous-integration/travis-ci/pr" is failing
status "branch manager" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

mmalerba · 2018-05-10T23:42:39Z

@jelbourn I moved the @Output() back outside the zone for now. This can be re-presubmitted

angular-automatic-lock-bot · 2019-09-08T23:02:29Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mmalerba added the pr: needs review label May 4, 2018

mmalerba assigned jelbourn May 4, 2018

mmalerba requested a review from jelbourn May 4, 2018 22:17

mmalerba requested a review from crisbeto as a code owner May 4, 2018 22:17

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 4, 2018

mmalerba force-pushed the obs-con branch from 375bb52 to 503dab3 Compare May 4, 2018 23:24

crisbeto reviewed May 5, 2018

View reviewed changes

mmalerba force-pushed the obs-con branch 2 times, most recently from 82e86d9 to 492e9fb Compare May 8, 2018 22:35

jelbourn reviewed May 9, 2018

View reviewed changes

mmalerba force-pushed the obs-con branch 2 times, most recently from 42c014d to 45cc979 Compare May 9, 2018 18:51

jelbourn approved these changes May 9, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed pr: needs review labels May 9, 2018

mmalerba added 4 commits May 10, 2018 16:28

feat(observe-content): refactor so logic can be used without directive

c3f7831

address comments

86edf39

address comments

7d8ab68

move @Ouput() back outside the zone to prevent breaking changes

691a7ad

mmalerba force-pushed the obs-con branch from 45cc979 to 691a7ad Compare May 10, 2018 23:41

tinayuangao merged commit ba57852 into angular:master May 15, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(observe-content): refactor so logic can be used without directive #11170

feat(observe-content): refactor so logic can be used without directive #11170

mmalerba commented May 4, 2018

crisbeto May 5, 2018

mmalerba May 5, 2018

crisbeto May 5, 2018

jelbourn May 7, 2018

crisbeto May 5, 2018

mmalerba May 5, 2018

mmalerba commented May 8, 2018

jelbourn May 9, 2018

mmalerba May 9, 2018

jelbourn May 9, 2018

jelbourn May 9, 2018

mmalerba May 9, 2018

jelbourn May 9, 2018

jelbourn May 9, 2018

mmalerba commented May 9, 2018

jelbourn left a comment

ngbot bot commented May 9, 2018

mmalerba commented May 10, 2018

angular-automatic-lock-bot bot commented Sep 8, 2019



		/** A class that observes an element for content changes. */
		export class ContentObserver {


		constructor(private _mutationObserverFactory: MutationObserverFactory) {}

		observe(element: Element, debounce?: number): Observable<MutationRecord[]> {

feat(observe-content): refactor so logic can be used without directive #11170

feat(observe-content): refactor so logic can be used without directive #11170

Conversation

mmalerba commented May 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented May 9, 2018

jelbourn left a comment

Choose a reason for hiding this comment

ngbot bot commented May 9, 2018

mmalerba commented May 10, 2018

angular-automatic-lock-bot bot commented Sep 8, 2019