Skip to content

Commit 195c5c2

Browse files
vsavkinvsavkin
committed
fix(change_detection): update the right change detector when using ON_PUSH mode
Previously, in a case where you have a mix of ON_PUSH and DEFAULT detectors, Angular would update the status of a wrong detector.
1 parent a0b2408 commit 195c5c2

7 files changed

Lines changed: 53 additions & 54 deletions

File tree

‎modules/angular2/src/change_detection/abstract_change_detector.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,10 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
210210
return value;
211211
}
212212

213+
protected getDetectorFor(directives: any, index: number): ChangeDetector {
214+
return directives.getDetectorFor(this.directiveRecords[index].directiveIndex);
215+
}
216+
213217
protected notifyDispatcher(value: any): void {
214218
this.dispatcher.notifyOnBinding(this._currentBinding(), value);
215219
}

‎modules/angular2/src/change_detection/change_detection_jit_generator.ts‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ export class ChangeDetectorJITGenerator {
143143

144144
_maybeGenHydrateDirectives(): string {
145145
var hydrateDirectivesCode = this._genHydrateDirectives();
146-
var hydrateDetectorsCode = this._genHydrateDetectors();
146+
var hydrateDetectorsCode = this._logic.genHydrateDetectors(this.directiveRecords);
147147
if (!hydrateDirectivesCode && !hydrateDetectorsCode) return '';
148148
return `${this._typeName}.prototype.hydrateDirectives = function(directives) {
149149
${hydrateDirectivesCode}
@@ -161,16 +161,6 @@ export class ChangeDetectorJITGenerator {
161161
return lines.join('\n');
162162
}
163163

164-
_genHydrateDetectors(): string {
165-
var detectorFieldNames = this._names.getAllDetectorNames();
166-
var lines = ListWrapper.createFixedSize(detectorFieldNames.length);
167-
for (var i = 0, iLen = detectorFieldNames.length; i < iLen; ++i) {
168-
lines[i] = `${detectorFieldNames[i]} = directives.getDetectorFor(
169-
${this._names.getDirectivesAccessorName()}[${i}].directiveIndex);`;
170-
}
171-
return lines.join('\n');
172-
}
173-
174164
_maybeGenCallOnAllChangesDone(): string {
175165
var notifications = [];
176166
var dirs = this.directiveRecords;

‎modules/angular2/src/change_detection/codegen_logic_util.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {BaseException, Json, StringWrapper} from 'angular2/src/facade/lang';
33
import {CodegenNameUtil} from './codegen_name_util';
44
import {codify, combineGeneratedStrings, rawString} from './codegen_facade';
55
import {ProtoRecord, RecordType} from './proto_record';
6+
import {DirectiveRecord} from './directive_record';
67

78
/**
89
* This is an experimental feature. Works only in Dart.
@@ -131,4 +132,16 @@ export class CodegenLogicUtil {
131132
iVals.push(codify(protoRec.fixedArgs[protoRec.args.length]));
132133
return combineGeneratedStrings(iVals);
133134
}
135+
136+
genHydrateDetectors(directiveRecords: DirectiveRecord[]): string {
137+
var res = [];
138+
for (var i = 0; i < directiveRecords.length; ++i) {
139+
var r = directiveRecords[i];
140+
if (!r.isDefaultChangeDetection()) {
141+
res.push(
142+
`${this._names.getDetectorName(r.directiveIndex)} = this.getDetectorFor(directives, ${i});`);
143+
}
144+
}
145+
return res.join("\n");
146+
}
134147
}

‎modules/angular2/src/change_detection/codegen_name_util.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -200,11 +200,5 @@ export class CodegenNameUtil {
200200
return this._addFieldPrefix(`directive_${d.name}`);
201201
}
202202

203-
getAllDetectorNames(): List<string> {
204-
return ListWrapper.map(
205-
ListWrapper.filter(this.directiveRecords, r => !r.isDefaultChangeDetection()),
206-
(d) => this.getDetectorName(d.directiveIndex));
207-
}
208-
209203
getDetectorName(d: DirectiveIndex): string { return this._addFieldPrefix(`detector_${d.name}`); }
210204
}

‎modules/angular2/src/transform/template_compiler/change_detector_codegen.dart‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ class _CodegenState {
226226

227227
String _maybeGenHydrateDirectives() {
228228
var hydrateDirectivesCode = _genHydrateDirectives();
229-
var hydrateDetectorsCode = _genHydrateDetectors();
229+
var hydrateDetectorsCode = _logic.genHydrateDetectors(_directiveRecords);
230230
if (hydrateDirectivesCode.isEmpty && hydrateDetectorsCode.isEmpty) {
231231
return '';
232232
}
@@ -244,16 +244,6 @@ class _CodegenState {
244244
return '$buf';
245245
}
246246

247-
String _genHydrateDetectors() {
248-
var buf = new StringBuffer();
249-
var detectorFieldNames = _names.getAllDetectorNames();
250-
for (var i = 0; i < detectorFieldNames.length; ++i) {
251-
buf.writeln('${detectorFieldNames[i]} = directives.getDetectorFor('
252-
'${_names.getDirectivesAccessorName()}[$i].directiveIndex);');
253-
}
254-
return '$buf';
255-
}
256-
257247
/// Generates calls to `onAllChangesDone` for all `Directive`s that request
258248
/// them.
259249
String _maybeGenCallOnAllChangesDone() {

‎modules/angular2/test/change_detection/change_detector_config.ts‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -183,24 +183,25 @@ class _ExpressionWithMode {
183183
var directiveRecords = [];
184184
var eventRecords = [];
185185

186+
var dirRecordWithDefault =
187+
new DirectiveRecord({directiveIndex: new DirectiveIndex(0, 0), changeDetection: DEFAULT});
188+
var dirRecordWithOnPush =
189+
new DirectiveRecord({directiveIndex: new DirectiveIndex(0, 1), changeDetection: ON_PUSH});
190+
186191
if (this._withRecords) {
187-
var dirRecordWithOnPush =
188-
new DirectiveRecord({directiveIndex: new DirectiveIndex(0, 0), changeDetection: ON_PUSH});
192+
var updateDirWithOnDefaultRecord =
193+
BindingRecord.createForDirective(_getParser().parseBinding('42', 'location'), 'a',
194+
(o, v) => (<any>o).a = v, dirRecordWithDefault);
189195
var updateDirWithOnPushRecord =
190196
BindingRecord.createForDirective(_getParser().parseBinding('42', 'location'), 'a',
191197
(o, v) => (<any>o).a = v, dirRecordWithOnPush);
192-
bindingRecords = [updateDirWithOnPushRecord];
193-
directiveRecords = [dirRecordWithOnPush];
194-
} else {
195-
bindingRecords = [];
196-
directiveRecords = [];
198+
199+
directiveRecords = [dirRecordWithDefault, dirRecordWithOnPush];
200+
bindingRecords = [updateDirWithOnDefaultRecord, updateDirWithOnPushRecord];
197201
}
198202

199203
if (this._withEvents) {
200-
var dirRecordWithOnPush =
201-
new DirectiveRecord({directiveIndex: new DirectiveIndex(0, 0), changeDetection: ON_PUSH});
202-
directiveRecords = [dirRecordWithOnPush];
203-
204+
directiveRecords = [dirRecordWithDefault, dirRecordWithOnPush];
204205
eventRecords =
205206
ListWrapper.concat(_createEventRecords("(event)='false'"),
206207
_createHostEventRecords("(host-event)='false'", dirRecordWithOnPush))

‎modules/angular2/test/change_detection/change_detector_spec.ts‎

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -704,29 +704,36 @@ export function main() {
704704
});
705705

706706
describe('marking ON_PUSH detectors as CHECK_ONCE after an update', () => {
707-
var checkedDetector;
707+
var childDirectiveDetectorRegular;
708+
var childDirectiveDetectorOnPush;
708709
var directives;
709710

710711
beforeEach(() => {
711-
checkedDetector = _createWithoutHydrate('emptyUsingOnPushStrategy').changeDetector;
712-
checkedDetector.hydrate(_DEFAULT_CONTEXT, null, null, null);
713-
checkedDetector.mode = CHECKED;
714-
715-
var targetDirective = new TestData(null);
716-
directives = new FakeDirectives([targetDirective], [checkedDetector]);
712+
childDirectiveDetectorRegular = _createWithoutHydrate('10').changeDetector;
713+
childDirectiveDetectorRegular.hydrate(_DEFAULT_CONTEXT, null, null, null);
714+
childDirectiveDetectorRegular.mode = CHECK_ALWAYS;
715+
716+
childDirectiveDetectorOnPush =
717+
_createWithoutHydrate('emptyUsingOnPushStrategy').changeDetector;
718+
childDirectiveDetectorOnPush.hydrate(_DEFAULT_CONTEXT, null, null, null);
719+
childDirectiveDetectorOnPush.mode = CHECKED;
720+
721+
directives =
722+
new FakeDirectives([new TestData(null), new TestData(null)],
723+
[childDirectiveDetectorRegular, childDirectiveDetectorOnPush]);
717724
});
718725

719726
it('should set the mode to CHECK_ONCE when a binding is updated', () => {
720-
var cd = _createWithoutHydrate('onPushRecordsUsingDefaultStrategy').changeDetector;
721-
cd.hydrate(_DEFAULT_CONTEXT, null, directives, null);
727+
var parentDetector =
728+
_createWithoutHydrate('onPushRecordsUsingDefaultStrategy').changeDetector;
729+
parentDetector.hydrate(_DEFAULT_CONTEXT, null, directives, null);
722730

723-
expect(checkedDetector.mode).toEqual(CHECKED);
731+
parentDetector.detectChanges();
724732

725-
// evaluate the record, update the targetDirective, and mark its detector as
726-
// CHECK_ONCE
727-
cd.detectChanges();
733+
// making sure that we only change the status of ON_PUSH components
734+
expect(childDirectiveDetectorRegular.mode).toEqual(CHECK_ALWAYS);
728735

729-
expect(checkedDetector.mode).toEqual(CHECK_ONCE);
736+
expect(childDirectiveDetectorOnPush.mode).toEqual(CHECK_ONCE);
730737
});
731738

732739
it('should mark ON_PUSH detectors as CHECK_ONCE after an event', () => {
@@ -745,7 +752,7 @@ export function main() {
745752

746753
cd.handleEvent("host-event", 0, null);
747754

748-
expect(checkedDetector.mode).toEqual(CHECK_ONCE);
755+
expect(childDirectiveDetectorOnPush.mode).toEqual(CHECK_ONCE);
749756
});
750757

751758
if (IS_DART) {

0 commit comments

Comments
 (0)