virtual-scroll: use `detectChanges` rather than `markForCheck` in `CdkVirtualScrollViewport` #12158

mmalerba · 2018-07-11T17:26:58Z

No description provided.

`CdkVirtualScrollViewport`

mmalerba · 2018-07-11T17:35:02Z

So theoretically this should be a little better for performance, I think.. markForCheck checks from the root and detectChanges just checks the component you call it on. All of the examples I have in the demo app still seem to work fine after this change

crisbeto

LGTM

crisbeto · 2018-07-11T17:45:01Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

+    this._ngZone.runOutsideAngular(() => {
+      this._forOf = forOf;
+      this._forOf.dataStream.pipe(takeUntil(this._detachedSubject)).subscribe(data => {
+        const len = data.length;


Should this be called something like newLength?

ngbot · 2018-07-11T17:46:28Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
missing required label: "target: *"
status "continuous-integration/travis-ci/pr" 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.

jelbourn · 2018-07-11T18:29:01Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

  constructor(public elementRef: ElementRef, private _changeDetectorRef: ChangeDetectorRef,
              private _ngZone: NgZone, private _sanitizer: DomSanitizer,
              @Inject(VIRTUAL_SCROLL_STRATEGY) private _scrollStrategy: VirtualScrollStrategy) {}

  ngOnInit() {
    // It's still too early to measure the viewport at this point. Deferring with a promise allows
    // the Viewport to be rendered with the correct size before we measure.
-    Promise.resolve().then(() => {
+    this._ngZone.runOutsideAngular(() => Promise.resolve().then(() => {


Expand comment to explain why you run outside the zone?

jelbourn · 2018-07-11T18:29:21Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

@@ -102,37 +102,29 @@ export class CdkVirtualScrollViewport implements DoCheck, OnInit, OnDestroy {
  /** Observable that emits when the viewport is destroyed. */
  private _destroyed = new Subject<void>();

+  /** Whether there is a pending change detection cycle. */
+  private _checkPending = false;


_isPendingChangeDetection or _needsChangeDetection?

jelbourn · 2018-07-11T22:00:47Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

-        this._dataLength = len;
-        this._scrollStrategy.onDataLengthChanged();
-      }
+    this._ngZone.runOutsideAngular(() => {


Add comment that explains why this is going out of the zone?

jelbourn · 2018-07-11T22:02:32Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

+  private _doCheck() {
+    this._checkPending = false;
+    this._ngZone.run(() => this._changeDetectorRef.detectChanges());
+    // In order to batch setting the scroll offset together with other DOM writes, we wait until a


I don't quite follow what this comment means about waiting, since detectChanges is synchronous

Yes, but in the _markForCheck method above I'm batching them up using a Promise.resolve(). This way if we change multiple properties it waits until after they're all updated and calls detectChanges once

Should the comment live where the wait is scheduled?

I split the comment up to explain the batching part where it actually happens

jelbourn · 2018-07-11T22:03:56Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

+  }
+
+  /** Run change detection. */
+  private _doCheck() {


Call this method something like _detectChangesForScroll?

jelbourn

LGTM

amcdnl · 2018-07-15T17:56:14Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.ts

+      }
+    }
+
+    for (let fn of this._runAfterChangeDetection) {


nit: use const instead of let

👍 added fix to #12183

amcdnl

LGTM

naveedahmed1 · 2018-07-20T19:27:53Z

Is it merged in 6.4.0? I am using virtual-scroll in a component with changeDetection: ChangeDetectionStrategy.OnPush which was working properly in 6.3.3. With 6.4.0 all I see is a blank space until I remove `changeDetection: ChangeDetectionStrategy.OnPush'

mmalerba · 2018-07-23T16:50:26Z

@naveedahmed1 You are right, it looks like this broke it for parents that are using OnPush strategy. I will create a fix

…12329) breaks viewports that are inside an `OnPush` component. Fixes issue introduced by #12158

…kVirtualScrollViewport` (#12158)

…12329) breaks viewports that are inside an `OnPush` component. Fixes issue introduced by #12158

angular-automatic-lock-bot · 2019-09-09T07:02:50Z

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.}

virtual-scroll: use detectChanges rather than markForCheck in

8438ba8

`CdkVirtualScrollViewport`

mmalerba assigned amcdnl, jelbourn and crisbeto Jul 11, 2018

mmalerba requested review from amcdnl, jelbourn and crisbeto July 11, 2018 17:26

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

crisbeto approved these changes Jul 11, 2018

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 11, 2018

fix lint and tests

38afb9e

mmalerba added the target: master only label Jul 11, 2018

mmalerba force-pushed the vs-mfc branch from 91ba2c1 to 66856a7 Compare July 11, 2018 21:10

jelbourn reviewed Jul 11, 2018

View reviewed changes

mmalerba force-pushed the vs-mfc branch from 66856a7 to 0ebee9a Compare July 11, 2018 22:41

address comments

138e2be

mmalerba force-pushed the vs-mfc branch from 0ebee9a to 138e2be Compare July 11, 2018 22:47

mmalerba added the pr: merge safe label Jul 11, 2018

jelbourn approved these changes Jul 11, 2018

View reviewed changes

jelbourn merged commit 3255cf3 into angular:master Jul 12, 2018

amcdnl reviewed Jul 15, 2018

View reviewed changes

mmalerba mentioned this pull request Jul 23, 2018

virtual-scroll: switch back to markForCheck since detectChanges #12329

Merged

mmalerba added a commit that referenced this pull request Jul 24, 2018

virtual-scroll: switch back to markForCheck since detectChanges (#…

e67fd7b

…12329) breaks viewports that are inside an `OnPush` component. Fixes issue introduced by #12158

victoriaaa234 pushed a commit that referenced this pull request Jul 25, 2018

virtual-scroll: use detectChanges rather than markForCheck in `Cd…

7000c93

…kVirtualScrollViewport` (#12158)

mmalerba added a commit that referenced this pull request Jul 30, 2018

virtual-scroll: switch back to markForCheck since detectChanges (#…

aae045c

…12329) breaks viewports that are inside an `OnPush` component. Fixes issue introduced by #12158

angular-automatic-lock-bot bot unassigned amcdnl Sep 9, 2019

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

virtual-scroll: use detectChanges rather than markForCheck in CdkVirtualScrollViewport #12158

virtual-scroll: use detectChanges rather than markForCheck in CdkVirtualScrollViewport #12158

Uh oh!

Conversation

mmalerba commented Jul 11, 2018

Uh oh!

mmalerba commented Jul 11, 2018

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcdnl left a comment

Choose a reason for hiding this comment

Uh oh!

naveedahmed1 commented Jul 20, 2018

Uh oh!

mmalerba commented Jul 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

virtual-scroll: use `detectChanges` rather than `markForCheck` in `CdkVirtualScrollViewport` #12158

virtual-scroll: use `detectChanges` rather than `markForCheck` in `CdkVirtualScrollViewport` #12158