Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8c82106

Browse filesBrowse files
JiaLiPassionjosephperrott
JiaLiPassion
authored and
josephperrott
committed
fix(core): markDirty() should only mark flags when really scheduling tick. (#39316)
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`. PR Close #39316
1 parent fb96c3d commit 8c82106
Copy full SHA for 8c82106

File tree

Expand file treeCollapse file tree

2 files changed

+104
-5
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+104
-5
lines changed
Open diff view settings
Collapse file

‎packages/core/src/render3/instructions/shared.ts‎

Copy file name to clipboardExpand all lines: packages/core/src/render3/instructions/shared.ts
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1863,9 +1863,10 @@ export function markViewDirty(lView: LView): LView|null {
18631863
*/
18641864
export function scheduleTick(rootContext: RootContext, flags: RootContextFlags) {
18651865
const nothingScheduled = rootContext.flags === RootContextFlags.Empty;
1866-
rootContext.flags |= flags;
1867-
18681866
if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) {
1867+
// https://github.com/angular/angular/issues/39296
1868+
// should only attach the flags when really scheduling a tick
1869+
rootContext.flags |= flags;
18691870
let res: null|((val: null) => void);
18701871
rootContext.clean = new Promise<null>((r) => res = r);
18711872
rootContext.scheduler(() => {
Collapse file

‎packages/core/test/render3/change_detection_spec.ts‎

Copy file name to clipboardExpand all lines: packages/core/test/render3/change_detection_spec.ts
+101-3Lines changed: 101 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,20 @@
88

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

11-
import {ChangeDetectionStrategy, DoCheck} from '../../src/core';
11+
import {ChangeDetectionStrategy, DoCheck, OnInit} from '../../src/core';
1212
import {whenRendered} from '../../src/render3/component';
13-
import {getRenderedText, LifecycleHooksFeature, ɵɵadvance, ɵɵdefineComponent, ɵɵgetCurrentView, ɵɵproperty, ɵɵtextInterpolate1, ɵɵtextInterpolate2} from '../../src/render3/index';
14-
import {detectChanges, markDirty, tick, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵlistener, ɵɵtext, ɵɵtextInterpolate} from '../../src/render3/instructions/all';
13+
import {AttributeMarker, getRenderedText, LifecycleHooksFeature, ɵɵadvance, ɵɵdefineComponent, ɵɵgetCurrentView, ɵɵproperty, ɵɵtextInterpolate1, ɵɵtextInterpolate2} from '../../src/render3/index';
14+
import {detectChanges, markDirty, tick, ɵɵelement, ɵɵelementEnd, ɵɵelementStart, ɵɵlistener, ɵɵtemplate, ɵɵtext, ɵɵtextInterpolate} from '../../src/render3/instructions/all';
1515
import {RenderFlags} from '../../src/render3/interfaces/definition';
1616
import {Renderer3, RendererFactory3} from '../../src/render3/interfaces/renderer';
1717
import {FLAGS, LViewFlags} from '../../src/render3/interfaces/view';
1818

19+
import {NgIf} from './common_with_def';
1920
import {containerEl, createComponent, renderComponent, requestAnimationFrame} from './render_util';
2021

2122
describe('change detection', () => {
2223
describe('markDirty, detectChanges, whenRendered, getRenderedText', () => {
24+
let mycompOninit: MyComponentWithOnInit;
2325
class MyComponent implements DoCheck {
2426
value: string = 'works';
2527
doCheckCount = 0;
@@ -48,6 +50,84 @@ describe('change detection', () => {
4850
});
4951
}
5052

53+
class MyComponentWithOnInit implements OnInit, DoCheck {
54+
value: string = 'works';
55+
doCheckCount = 0;
56+
57+
ngOnInit() {
58+
markDirty(this);
59+
}
60+
61+
ngDoCheck(): void {
62+
this.doCheckCount++;
63+
}
64+
65+
click() {
66+
this.value = 'click works';
67+
markDirty(this);
68+
}
69+
70+
static ɵfac = () => mycompOninit = new MyComponentWithOnInit();
71+
static ɵcmp = ɵɵdefineComponent({
72+
type: MyComponentWithOnInit,
73+
selectors: [['my-comp-oninit']],
74+
decls: 2,
75+
vars: 1,
76+
template:
77+
(rf: RenderFlags, ctx: MyComponentWithOnInit) => {
78+
if (rf & RenderFlags.Create) {
79+
ɵɵelementStart(0, 'span');
80+
ɵɵtext(1);
81+
ɵɵelementEnd();
82+
}
83+
if (rf & RenderFlags.Update) {
84+
ɵɵadvance(1);
85+
ɵɵtextInterpolate(ctx.value);
86+
}
87+
}
88+
});
89+
}
90+
91+
class MyParentComponent implements OnInit {
92+
show = false;
93+
value = 'parent';
94+
mycomp: any = undefined;
95+
96+
ngOnInit() {}
97+
98+
click() {
99+
this.show = true;
100+
markDirty(this);
101+
}
102+
103+
static ɵfac = () => new MyParentComponent();
104+
static ɵcmp = ɵɵdefineComponent({
105+
type: MyParentComponent,
106+
selectors: [['my-parent-comp']],
107+
decls: 2,
108+
vars: 1,
109+
directives: [NgIf, MyComponentWithOnInit],
110+
consts: [[AttributeMarker.Template, 'ngIf']],
111+
template:
112+
(rf: RenderFlags, ctx: MyParentComponent) => {
113+
if (rf & RenderFlags.Create) {
114+
ɵɵtext(0, ' -->\n');
115+
ɵɵtemplate(1, (rf, ctx) => {
116+
if (rf & RenderFlags.Create) {
117+
ɵɵelementStart(0, 'div');
118+
ɵɵelement(1, 'my-comp-oninit');
119+
ɵɵelementEnd();
120+
}
121+
}, 2, 0, 'div', 0);
122+
}
123+
if (rf & RenderFlags.Update) {
124+
ɵɵadvance(1);
125+
ɵɵproperty('ngIf', ctx.show);
126+
}
127+
}
128+
});
129+
}
130+
51131
it('should mark a component dirty and schedule change detection', withBody('my-comp', () => {
52132
const myComp = renderComponent(MyComponent, {hostFeatures: [LifecycleHooksFeature]});
53133
expect(getRenderedText(myComp)).toEqual('works');
@@ -66,6 +146,24 @@ describe('change detection', () => {
66146
expect(getRenderedText(myComp)).toEqual('updated');
67147
}));
68148

149+
it('should detectChanges after markDirty is called multiple times within ngOnInit',
150+
withBody('my-comp-oninit', () => {
151+
const myParentComp =
152+
renderComponent(MyParentComponent, {hostFeatures: [LifecycleHooksFeature]});
153+
expect(myParentComp.show).toBe(false);
154+
myParentComp.click();
155+
requestAnimationFrame.flush();
156+
expect(myParentComp.show).toBe(true);
157+
const myComp = mycompOninit;
158+
expect(getRenderedText(myComp)).toEqual('works');
159+
expect(myComp.doCheckCount).toBe(1);
160+
myComp.click();
161+
expect(getRenderedText(myComp)).toEqual('works');
162+
requestAnimationFrame.flush();
163+
expect(getRenderedText(myComp)).toEqual('click works');
164+
expect(myComp.doCheckCount).toBe(2);
165+
}));
166+
69167
it('should detectChanges only once if markDirty is called multiple times',
70168
withBody('my-comp', () => {
71169
const myComp = renderComponent(MyComponent, {hostFeatures: [LifecycleHooksFeature]});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.