Skip to content

Commit

Permalink
fix(core): markDirty() should only mark flags when really scheduling …
Browse files Browse the repository at this point in the history
…tick.

Close #39296

Fix an issue that `markDirty()` will not trigger change detection.

The case is for example we have the following component.

```
export class AppComponent implements OnInit {
  constructor(private router: Router) {}

  ngOnInit() {
    this.router.events
      .pipe(filter((e) => e instanceof NavigationEnd))
      .subscribe(() => ɵmarkDirty(this));
  }
}

export class CounterComponent implements OnInit, OnDestroy {
  ngOnInit() {
    this.countSubject.pipe(takeUntil(this.destroy)).subscribe((count) => {
      this.count = count;
      ɵmarkDirty(this);
    });
  }
```

Then the app navigate from `AppComponent` to `CounterComponent`,
so there are 2 `markDirty()` call at in a row.

The `1st` call is from `AppComponent` when router changed, the
`2nd` call is from `CounterComponent.ngOnInit()`.

And the `markDirty()->scheduleTick()` code look like this

```
function scheduleTick(rootContext, flags) {
    const nothingScheduled = rootContext.flags === 0 /* Empty */;
    rootContext.flags |= flags;
    if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) {
      rootContext.schedule(() => {
	...
        if (rootContext.flags & RootContextFlags.DetectChanges)
          rootContext.flags &= ~RootContextFlags.DetectChanges;
          tickContext();

        rootContext.clean = _CLEAN_PROMISE;
        ...
      });
```

So in this case, the `1st` markDirty() will
1. set rootContext.flags = 1
2. before `tickContext()`, reset rootContext.flags = 0
3. inside `tickContext()`, it will call `CounterComponent.ngOnint()`,
   so the `2nd` markDirty() is called.
4. and the `2nd` scheduleTick is called, `nothingScheduled` is true,
   but rootContext.clean is not `_CLEAN_PROMISE` yet, since the `1st` markDirty tick
   is still running.
5. So nowhere will reset the `rootContext.flags`.
6. then in the future, any other `markDirty()` call will not trigger the tick, since
   `nothingScheduled` is always false.

So `nothingScheduled` means no tick is scheduled, `rootContext.clean === _CLEAN_PROMISE`
means no tick is running.
So we should set the flags to `rootContext` only when `no tick is scheudled or running`.
  • Loading branch information
JiaLiPassion committed Oct 17, 2020
1 parent 79d67dc commit 6184918
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 3 deletions.
5 changes: 3 additions & 2 deletions packages/core/src/render3/instructions/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1863,9 +1863,10 @@ export function markViewDirty(lView: LView): LView|null {
*/
export function scheduleTick(rootContext: RootContext, flags: RootContextFlags) {
const nothingScheduled = rootContext.flags === RootContextFlags.Empty;
rootContext.flags |= flags;

if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) {
// https://github.com/angular/angular/issues/39296
// should only attach the flags when really scheduling a tick
rootContext.flags |= flags;
let res: null|((val: null) => void);
rootContext.clean = new Promise<null>((r) => res = r);
rootContext.scheduler(() => {
Expand Down
53 changes: 52 additions & 1 deletion packages/core/test/render3/change_detection_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import {withBody} from '@angular/private/testing';

import {ChangeDetectionStrategy, DoCheck} from '../../src/core';
import {ChangeDetectionStrategy, DoCheck, OnInit} from '../../src/core';
import {whenRendered} from '../../src/render3/component';
import {getRenderedText, LifecycleHooksFeature, ɵɵadvance, ɵɵdefineComponent, ɵɵgetCurrentView, ɵɵproperty, ɵɵtextInterpolate1, ɵɵtextInterpolate2} from '../../src/render3/index';
import {detectChanges, markDirty, tick, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵlistener, ɵɵtext, ɵɵtextInterpolate} from '../../src/render3/instructions/all';
Expand Down Expand Up @@ -48,6 +48,44 @@ describe('change detection', () => {
});
}

class MyComponentWithOnInit implements OnInit, DoCheck {
value: string = 'works';
doCheckCount = 0;

ngOnInit() {
markDirty(this);
}

ngDoCheck(): void {
this.doCheckCount++;
}

click() {
this.value = 'click works';
markDirty(this);
}

static ɵfac = () => new MyComponentWithOnInit();
static ɵcmp = ɵɵdefineComponent({
type: MyComponentWithOnInit,
selectors: [['my-comp-oninit']],
decls: 2,
vars: 1,
template:
(rf: RenderFlags, ctx: MyComponentWithOnInit) => {
if (rf & RenderFlags.Create) {
ɵɵelementStart(0, 'span');
ɵɵtext(1);
ɵɵelementEnd();
}
if (rf & RenderFlags.Update) {
ɵɵadvance(1);
ɵɵtextInterpolate(ctx.value);
}
}
});
}

it('should mark a component dirty and schedule change detection', withBody('my-comp', () => {
const myComp = renderComponent(MyComponent, {hostFeatures: [LifecycleHooksFeature]});
expect(getRenderedText(myComp)).toEqual('works');
Expand All @@ -66,6 +104,19 @@ describe('change detection', () => {
expect(getRenderedText(myComp)).toEqual('updated');
}));

it('should detectChanges after markDirty is called multiple times within ngOnInit',
withBody('my-comp-oninit', () => {
const myComp =
renderComponent(MyComponentWithOnInit, {hostFeatures: [LifecycleHooksFeature]});
expect(getRenderedText(myComp)).toEqual('works');
expect(myComp.doCheckCount).toBe(1);
myComp.click();
expect(getRenderedText(myComp)).toEqual('works');
requestAnimationFrame.flush();
expect(getRenderedText(myComp)).toEqual('click works');
expect(myComp.doCheckCount).toBe(2);
}));

it('should detectChanges only once if markDirty is called multiple times',
withBody('my-comp', () => {
const myComp = renderComponent(MyComponent, {hostFeatures: [LifecycleHooksFeature]});
Expand Down

0 comments on commit 6184918

Please sign in to comment.