From 195c5c21d434e8ab7c668dbb75dc2745f7be48ee Mon Sep 17 00:00:00 2001 From: vsavkin Date: Wed, 19 Aug 2015 10:48:53 -0700 Subject: [PATCH] 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. --- .../abstract_change_detector.ts | 4 +++ .../change_detection_jit_generator.ts | 12 +------ .../change_detection/codegen_logic_util.ts | 13 +++++++ .../src/change_detection/codegen_name_util.ts | 6 ---- .../change_detector_codegen.dart | 12 +------ .../change_detector_config.ts | 23 ++++++------ .../change_detection/change_detector_spec.ts | 35 +++++++++++-------- 7 files changed, 52 insertions(+), 53 deletions(-) diff --git a/modules/angular2/src/change_detection/abstract_change_detector.ts b/modules/angular2/src/change_detection/abstract_change_detector.ts index aac85baf53..82efc68b46 100644 --- a/modules/angular2/src/change_detection/abstract_change_detector.ts +++ b/modules/angular2/src/change_detection/abstract_change_detector.ts @@ -210,6 +210,10 @@ export class AbstractChangeDetector implements ChangeDetector { return value; } + protected getDetectorFor(directives: any, index: number): ChangeDetector { + return directives.getDetectorFor(this.directiveRecords[index].directiveIndex); + } + protected notifyDispatcher(value: any): void { this.dispatcher.notifyOnBinding(this._currentBinding(), value); } diff --git a/modules/angular2/src/change_detection/change_detection_jit_generator.ts b/modules/angular2/src/change_detection/change_detection_jit_generator.ts index ba26018123..78609c0081 100644 --- a/modules/angular2/src/change_detection/change_detection_jit_generator.ts +++ b/modules/angular2/src/change_detection/change_detection_jit_generator.ts @@ -143,7 +143,7 @@ export class ChangeDetectorJITGenerator { _maybeGenHydrateDirectives(): string { var hydrateDirectivesCode = this._genHydrateDirectives(); - var hydrateDetectorsCode = this._genHydrateDetectors(); + var hydrateDetectorsCode = this._logic.genHydrateDetectors(this.directiveRecords); if (!hydrateDirectivesCode && !hydrateDetectorsCode) return ''; return `${this._typeName}.prototype.hydrateDirectives = function(directives) { ${hydrateDirectivesCode} @@ -161,16 +161,6 @@ export class ChangeDetectorJITGenerator { return lines.join('\n'); } - _genHydrateDetectors(): string { - var detectorFieldNames = this._names.getAllDetectorNames(); - var lines = ListWrapper.createFixedSize(detectorFieldNames.length); - for (var i = 0, iLen = detectorFieldNames.length; i < iLen; ++i) { - lines[i] = `${detectorFieldNames[i]} = directives.getDetectorFor( - ${this._names.getDirectivesAccessorName()}[${i}].directiveIndex);`; - } - return lines.join('\n'); - } - _maybeGenCallOnAllChangesDone(): string { var notifications = []; var dirs = this.directiveRecords; diff --git a/modules/angular2/src/change_detection/codegen_logic_util.ts b/modules/angular2/src/change_detection/codegen_logic_util.ts index 13842ee596..1181c82432 100644 --- a/modules/angular2/src/change_detection/codegen_logic_util.ts +++ b/modules/angular2/src/change_detection/codegen_logic_util.ts @@ -3,6 +3,7 @@ import {BaseException, Json, StringWrapper} from 'angular2/src/facade/lang'; import {CodegenNameUtil} from './codegen_name_util'; import {codify, combineGeneratedStrings, rawString} from './codegen_facade'; import {ProtoRecord, RecordType} from './proto_record'; +import {DirectiveRecord} from './directive_record'; /** * This is an experimental feature. Works only in Dart. @@ -131,4 +132,16 @@ export class CodegenLogicUtil { iVals.push(codify(protoRec.fixedArgs[protoRec.args.length])); return combineGeneratedStrings(iVals); } + + genHydrateDetectors(directiveRecords: DirectiveRecord[]): string { + var res = []; + for (var i = 0; i < directiveRecords.length; ++i) { + var r = directiveRecords[i]; + if (!r.isDefaultChangeDetection()) { + res.push( + `${this._names.getDetectorName(r.directiveIndex)} = this.getDetectorFor(directives, ${i});`); + } + } + return res.join("\n"); + } } diff --git a/modules/angular2/src/change_detection/codegen_name_util.ts b/modules/angular2/src/change_detection/codegen_name_util.ts index 8e99ec5d3f..4300c17ac9 100644 --- a/modules/angular2/src/change_detection/codegen_name_util.ts +++ b/modules/angular2/src/change_detection/codegen_name_util.ts @@ -200,11 +200,5 @@ export class CodegenNameUtil { return this._addFieldPrefix(`directive_${d.name}`); } - getAllDetectorNames(): List { - return ListWrapper.map( - ListWrapper.filter(this.directiveRecords, r => !r.isDefaultChangeDetection()), - (d) => this.getDetectorName(d.directiveIndex)); - } - getDetectorName(d: DirectiveIndex): string { return this._addFieldPrefix(`detector_${d.name}`); } } diff --git a/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart b/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart index d1a2b1ba6b..81a6e19e1b 100644 --- a/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart +++ b/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart @@ -226,7 +226,7 @@ class _CodegenState { String _maybeGenHydrateDirectives() { var hydrateDirectivesCode = _genHydrateDirectives(); - var hydrateDetectorsCode = _genHydrateDetectors(); + var hydrateDetectorsCode = _logic.genHydrateDetectors(_directiveRecords); if (hydrateDirectivesCode.isEmpty && hydrateDetectorsCode.isEmpty) { return ''; } @@ -244,16 +244,6 @@ class _CodegenState { return '$buf'; } - String _genHydrateDetectors() { - var buf = new StringBuffer(); - var detectorFieldNames = _names.getAllDetectorNames(); - for (var i = 0; i < detectorFieldNames.length; ++i) { - buf.writeln('${detectorFieldNames[i]} = directives.getDetectorFor(' - '${_names.getDirectivesAccessorName()}[$i].directiveIndex);'); - } - return '$buf'; - } - /// Generates calls to `onAllChangesDone` for all `Directive`s that request /// them. String _maybeGenCallOnAllChangesDone() { diff --git a/modules/angular2/test/change_detection/change_detector_config.ts b/modules/angular2/test/change_detection/change_detector_config.ts index baf8d81a1c..3442a1fef2 100644 --- a/modules/angular2/test/change_detection/change_detector_config.ts +++ b/modules/angular2/test/change_detection/change_detector_config.ts @@ -183,24 +183,25 @@ class _ExpressionWithMode { var directiveRecords = []; var eventRecords = []; + var dirRecordWithDefault = + new DirectiveRecord({directiveIndex: new DirectiveIndex(0, 0), changeDetection: DEFAULT}); + var dirRecordWithOnPush = + new DirectiveRecord({directiveIndex: new DirectiveIndex(0, 1), changeDetection: ON_PUSH}); + if (this._withRecords) { - var dirRecordWithOnPush = - new DirectiveRecord({directiveIndex: new DirectiveIndex(0, 0), changeDetection: ON_PUSH}); + var updateDirWithOnDefaultRecord = + BindingRecord.createForDirective(_getParser().parseBinding('42', 'location'), 'a', + (o, v) => (o).a = v, dirRecordWithDefault); var updateDirWithOnPushRecord = BindingRecord.createForDirective(_getParser().parseBinding('42', 'location'), 'a', (o, v) => (o).a = v, dirRecordWithOnPush); - bindingRecords = [updateDirWithOnPushRecord]; - directiveRecords = [dirRecordWithOnPush]; - } else { - bindingRecords = []; - directiveRecords = []; + + directiveRecords = [dirRecordWithDefault, dirRecordWithOnPush]; + bindingRecords = [updateDirWithOnDefaultRecord, updateDirWithOnPushRecord]; } if (this._withEvents) { - var dirRecordWithOnPush = - new DirectiveRecord({directiveIndex: new DirectiveIndex(0, 0), changeDetection: ON_PUSH}); - directiveRecords = [dirRecordWithOnPush]; - + directiveRecords = [dirRecordWithDefault, dirRecordWithOnPush]; eventRecords = ListWrapper.concat(_createEventRecords("(event)='false'"), _createHostEventRecords("(host-event)='false'", dirRecordWithOnPush)) diff --git a/modules/angular2/test/change_detection/change_detector_spec.ts b/modules/angular2/test/change_detection/change_detector_spec.ts index 34a8ff264c..e814617cd9 100644 --- a/modules/angular2/test/change_detection/change_detector_spec.ts +++ b/modules/angular2/test/change_detection/change_detector_spec.ts @@ -704,29 +704,36 @@ export function main() { }); describe('marking ON_PUSH detectors as CHECK_ONCE after an update', () => { - var checkedDetector; + var childDirectiveDetectorRegular; + var childDirectiveDetectorOnPush; var directives; beforeEach(() => { - checkedDetector = _createWithoutHydrate('emptyUsingOnPushStrategy').changeDetector; - checkedDetector.hydrate(_DEFAULT_CONTEXT, null, null, null); - checkedDetector.mode = CHECKED; + childDirectiveDetectorRegular = _createWithoutHydrate('10').changeDetector; + childDirectiveDetectorRegular.hydrate(_DEFAULT_CONTEXT, null, null, null); + childDirectiveDetectorRegular.mode = CHECK_ALWAYS; - var targetDirective = new TestData(null); - directives = new FakeDirectives([targetDirective], [checkedDetector]); + childDirectiveDetectorOnPush = + _createWithoutHydrate('emptyUsingOnPushStrategy').changeDetector; + childDirectiveDetectorOnPush.hydrate(_DEFAULT_CONTEXT, null, null, null); + childDirectiveDetectorOnPush.mode = CHECKED; + + directives = + new FakeDirectives([new TestData(null), new TestData(null)], + [childDirectiveDetectorRegular, childDirectiveDetectorOnPush]); }); it('should set the mode to CHECK_ONCE when a binding is updated', () => { - var cd = _createWithoutHydrate('onPushRecordsUsingDefaultStrategy').changeDetector; - cd.hydrate(_DEFAULT_CONTEXT, null, directives, null); + var parentDetector = + _createWithoutHydrate('onPushRecordsUsingDefaultStrategy').changeDetector; + parentDetector.hydrate(_DEFAULT_CONTEXT, null, directives, null); - expect(checkedDetector.mode).toEqual(CHECKED); + parentDetector.detectChanges(); - // evaluate the record, update the targetDirective, and mark its detector as - // CHECK_ONCE - cd.detectChanges(); + // making sure that we only change the status of ON_PUSH components + expect(childDirectiveDetectorRegular.mode).toEqual(CHECK_ALWAYS); - expect(checkedDetector.mode).toEqual(CHECK_ONCE); + expect(childDirectiveDetectorOnPush.mode).toEqual(CHECK_ONCE); }); it('should mark ON_PUSH detectors as CHECK_ONCE after an event', () => { @@ -745,7 +752,7 @@ export function main() { cd.handleEvent("host-event", 0, null); - expect(checkedDetector.mode).toEqual(CHECK_ONCE); + expect(childDirectiveDetectorOnPush.mode).toEqual(CHECK_ONCE); }); if (IS_DART) {