From b6e95bb96e617308e23e9bc7c52e2413195da10d Mon Sep 17 00:00:00 2001 From: Tim Blasi Date: Fri, 5 Jun 2015 17:33:51 -0700 Subject: [PATCH] feat(change detect): Throw on attempts to use dehydrated detector - Modify change detectors to `throw` when attempting to detect changes on a dehydrated detector. - Modify `DynamicChagneDetector` to use `null` for the `context` of a dehydrated detector. --- modules/angular2/change_detection.ts | 1 + .../change_detection_jit_generator.dart | 6 ++ .../change_detection_jit_generator.ts | 5 +- .../change_detection/change_detection_util.ts | 4 +- .../dynamic_change_detector.ts | 11 ++- .../src/change_detection/exceptions.ts | 6 +- .../change_detector_codegen.dart | 5 +- .../change_detection/change_detector_spec.ts | 67 ++++++++++++------- .../expected/bar.ng_deps.dart | 5 +- .../change_detection_benchmark.ts | 1 + 10 files changed, 78 insertions(+), 33 deletions(-) diff --git a/modules/angular2/change_detection.ts b/modules/angular2/change_detection.ts index deefc2a942..86e2e66db2 100644 --- a/modules/angular2/change_detection.ts +++ b/modules/angular2/change_detection.ts @@ -19,6 +19,7 @@ export {Parser} from './src/change_detection/parser/parser'; export {Locals} from './src/change_detection/parser/locals'; export { + DehydratedException, ExpressionChangedAfterItHasBeenChecked, ChangeDetectionError } from './src/change_detection/exceptions'; diff --git a/modules/angular2/src/change_detection/change_detection_jit_generator.dart b/modules/angular2/src/change_detection/change_detection_jit_generator.dart index 4147fda658..3b72191152 100644 --- a/modules/angular2/src/change_detection/change_detection_jit_generator.dart +++ b/modules/angular2/src/change_detection/change_detection_jit_generator.dart @@ -1,5 +1,11 @@ library change_detectoin.change_detection_jit_generator; +/// Placeholder JIT generator for Dart. +/// Dart does not support `eval`, so JIT generation is not an option. Instead, +/// the Dart transformer pre-generates these Change Detector classes and +/// registers them with the system. See `PreGeneratedChangeDetection`, +/// `PregenProtoChangeDetector`, and +/// `src/transform/template_compiler/change_detector_codegen.dart` for details. class ChangeDetectorJITGenerator { ChangeDetectorJITGenerator(typeName, strategy, records, directiveMementos) {} 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 ba63826aea..6aeedff4f9 100644 --- a/modules/angular2/src/change_detection/change_detection_jit_generator.ts +++ b/modules/angular2/src/change_detection/change_detection_jit_generator.ts @@ -98,6 +98,9 @@ export class ChangeDetectorJITGenerator { ${this.typeName}.prototype = Object.create(${ABSTRACT_CHANGE_DETECTOR}.prototype); ${this.typeName}.prototype.detectChangesInRecords = function(throwOnChange) { + if (!this.hydrated()) { + ${UTIL}.throwDehydrated(); + } ${this._genLocalDefinitions()} ${this._genChangeDefinitions()} var ${IS_CHANGED_LOCAL} = false; @@ -131,7 +134,7 @@ export class ChangeDetectorJITGenerator { } ${this.typeName}.prototype.hydrated = function() { - return Boolean(${CONTEXT_ACCESSOR}); + return ${CONTEXT_ACCESSOR} !== null; } return function(dispatcher, pipeRegistry) { diff --git a/modules/angular2/src/change_detection/change_detection_util.ts b/modules/angular2/src/change_detection/change_detection_util.ts index ea85820d98..413feac6f5 100644 --- a/modules/angular2/src/change_detection/change_detection_util.ts +++ b/modules/angular2/src/change_detection/change_detection_util.ts @@ -1,7 +1,7 @@ import {isPresent, isBlank, BaseException, Type} from 'angular2/src/facade/lang'; import {List, ListWrapper, MapWrapper, StringMapWrapper} from 'angular2/src/facade/collection'; import {ProtoRecord} from './proto_record'; -import {ExpressionChangedAfterItHasBeenChecked} from './exceptions'; +import {DehydratedException, ExpressionChangedAfterItHasBeenChecked} from './exceptions'; import {WrappedValue} from './pipes/pipe'; import {CHECK_ALWAYS, CHECK_ONCE, CHECKED, DETACHED, ON_PUSH} from './constants'; @@ -127,6 +127,8 @@ export class ChangeDetectionUtil { throw new ExpressionChangedAfterItHasBeenChecked(proto, change); } + static throwDehydrated() { throw new DehydratedException(); } + static changeDetectionMode(strategy: string) { return strategy == ON_PUSH ? CHECK_ONCE : CHECK_ALWAYS; } diff --git a/modules/angular2/src/change_detection/dynamic_change_detector.ts b/modules/angular2/src/change_detection/dynamic_change_detector.ts index 8bdcc6ff89..53a6dddb6f 100644 --- a/modules/angular2/src/change_detection/dynamic_change_detector.ts +++ b/modules/angular2/src/change_detection/dynamic_change_detector.ts @@ -44,7 +44,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector { this.prevContexts = ListWrapper.createFixedSize(protos.length + 1); this.changes = ListWrapper.createFixedSize(protos.length + 1); - ListWrapper.fill(this.values, uninitialized); + this.values[0] = null; + ListWrapper.fill(this.values, uninitialized, 1); ListWrapper.fill(this.pipes, null); ListWrapper.fill(this.prevContexts, uninitialized); ListWrapper.fill(this.changes, false); @@ -60,7 +61,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector { dehydrate() { this._destroyPipes(); - ListWrapper.fill(this.values, uninitialized); + this.values[0] = null; + ListWrapper.fill(this.values, uninitialized, 1); ListWrapper.fill(this.changes, false); ListWrapper.fill(this.pipes, null); ListWrapper.fill(this.prevContexts, uninitialized); @@ -75,9 +77,12 @@ export class DynamicChangeDetector extends AbstractChangeDetector { } } - hydrated(): boolean { return this.values[0] !== uninitialized; } + hydrated(): boolean { return this.values[0] !== null; } detectChangesInRecords(throwOnChange: boolean) { + if (!this.hydrated()) { + ChangeDetectionUtil.throwDehydrated(); + } var protos: List = this.protos; var changes = null; diff --git a/modules/angular2/src/change_detection/exceptions.ts b/modules/angular2/src/change_detection/exceptions.ts index 09fe1a9425..fbc34d8e7a 100644 --- a/modules/angular2/src/change_detection/exceptions.ts +++ b/modules/angular2/src/change_detection/exceptions.ts @@ -27,4 +27,8 @@ export class ChangeDetectionError extends BaseException { } toString(): string { return this.message; } -} \ No newline at end of file +} + +export class DehydratedException extends BaseException { + constructor() { super('Attempt to detect changes on a dehydrated detector.'); } +} 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 71afdf641a..cd9621d069 100644 --- a/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart +++ b/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart @@ -113,6 +113,9 @@ class _CodegenState { this.$_DIRECTIVES_ACCESSOR) : super(); void detectChangesInRecords(throwOnChange) { + if (!hydrated()) { + $_UTIL.throwDehydrated(); + } ${_genLocalDefinitions()} ${_genChangeDefinitons()} var $_IS_CHANGED_LOCAL = false; @@ -145,7 +148,7 @@ class _CodegenState { $_LOCALS_ACCESSOR = null; } - hydrated() => $_CONTEXT_ACCESSOR == null; + hydrated() => $_CONTEXT_ACCESSOR != null; static $_GEN_PREFIX.ProtoChangeDetector $PROTO_CHANGE_DETECTOR_FACTORY_METHOD( diff --git a/modules/angular2/test/change_detection/change_detector_spec.ts b/modules/angular2/test/change_detection/change_detector_spec.ts index 12ee689b1a..4beafcee9f 100644 --- a/modules/angular2/test/change_detection/change_detector_spec.ts +++ b/modules/angular2/test/change_detection/change_detector_spec.ts @@ -11,6 +11,7 @@ import { } from 'angular2/test_lib'; import { + CONST_EXPR, isPresent, isBlank, isJsObject, @@ -21,6 +22,7 @@ import {List, ListWrapper, MapWrapper, StringMapWrapper} from 'angular2/src/faca import { ChangeDispatcher, + DehydratedException, DynamicChangeDetector, ChangeDetectionError, BindingRecord, @@ -47,6 +49,7 @@ import { import {getDefinition} from './simple_watch_config'; import {getFactoryById} from './generated/simple_watch_classes'; +const _DEFAULT_CONTEXT = CONST_EXPR(new Object()); export function main() { // These tests also run against pre-generated Dart Change Detectors. We will move tests up from @@ -72,7 +75,7 @@ export function main() { } } - function _bindSimpleValue(expression: string, context = null) { + function _bindSimpleValue(expression: string, context = _DEFAULT_CONTEXT) { var dispatcher = new TestDispatcher(); var testDef = getDefinition(expression); var protoCd = _getProtoChangeDetector(testDef.cdDef); @@ -297,7 +300,7 @@ export function main() { function dirs(directives: List) { return new FakeDirectives(directives, []); } - function createChangeDetector(propName: string, exp: string, context = null, + function createChangeDetector(propName: string, exp: string, context = _DEFAULT_CONTEXT, registry = null) { var dispatcher = new TestDispatcher(); @@ -444,7 +447,7 @@ export function main() { var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive1])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive1])); cd.detectChanges(); @@ -465,7 +468,7 @@ export function main() { var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive1, directive2])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive1, directive2])); cd.detectChanges(); @@ -482,7 +485,7 @@ export function main() { var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive1])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive1])); cd.detectChanges(); @@ -502,7 +505,7 @@ export function main() { var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive1])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive1])); cd.checkNoChanges(); @@ -518,7 +521,7 @@ export function main() { var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive1])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive1])); cd.detectChanges(); @@ -538,7 +541,7 @@ export function main() { var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive1])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive1])); cd.checkNoChanges(); @@ -551,7 +554,7 @@ export function main() { var pcd = createProtoChangeDetector([], [], [dirRecord1, dirRecord2]); var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive1, directive2])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive1, directive2])); cd.detectChanges(); @@ -582,7 +585,7 @@ export function main() { var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive1])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive1])); cd.detectChanges(); @@ -599,7 +602,7 @@ export function main() { td1 = new TestDirective(() => ListWrapper.push(onChangesDoneCalls, td1)); var td2; td2 = new TestDirective(() => ListWrapper.push(onChangesDoneCalls, td2)); - cd.hydrate(null, null, dirs([td1, td2])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([td1, td2])); cd.detectChanges(); @@ -620,8 +623,8 @@ export function main() { var parentDirective = new TestDirective(() => { expect(directiveInShadowDom.a).toBe(null); }); - parent.hydrate(null, null, dirs([parentDirective])); - child.hydrate(null, null, dirs([directiveInShadowDom])); + parent.hydrate(_DEFAULT_CONTEXT, null, dirs([parentDirective])); + child.hydrate(_DEFAULT_CONTEXT, null, dirs([directiveInShadowDom])); parent.detectChanges(); }); @@ -641,7 +644,7 @@ export function main() { [BindingRecord.createForHostProperty(index, ast("a"), "prop")], null, [dirRecord]); var cd = pcd.instantiate(dispatcher); - cd.hydrate(null, null, dirs([directive])); + cd.hydrate(_DEFAULT_CONTEXT, null, dirs([directive])); cd.detectChanges(); @@ -688,7 +691,7 @@ export function main() { var cd = pcd.instantiate( new TestDispatcher(), [BindingRecord.createForElement(ast("invalidProp"), 0, "a")], null, []); - cd.hydrate(null, null); + cd.hydrate(_DEFAULT_CONTEXT, null); try { cd.detectChanges(); @@ -747,7 +750,7 @@ export function main() { expect(cd.mode).toEqual(null); - cd.hydrate(null, null, null); + cd.hydrate(_DEFAULT_CONTEXT, null, null); expect(cd.mode).toEqual(CHECK_ALWAYS); }); @@ -755,7 +758,7 @@ export function main() { it("should set the mode to CHECK_ONCE when the push change detection is used", () => { var proto = createProtoChangeDetector([], [], [], null, ON_PUSH); var cd = proto.instantiate(null); - cd.hydrate(null, null, null); + cd.hydrate(_DEFAULT_CONTEXT, null, null); expect(cd.mode).toEqual(CHECK_ONCE); }); @@ -765,6 +768,7 @@ export function main() { var cd = c["changeDetector"]; var dispatcher = c["dispatcher"]; + cd.hydrate(_DEFAULT_CONTEXT, null, null); cd.mode = DETACHED; cd.detectChanges(); @@ -776,6 +780,7 @@ export function main() { var cd = c["changeDetector"]; var dispatcher = c["dispatcher"]; + cd.hydrate(_DEFAULT_CONTEXT, null, null); cd.mode = CHECKED; cd.detectChanges(); @@ -784,6 +789,7 @@ export function main() { it("should change CHECK_ONCE to CHECKED", () => { var cd = createProtoChangeDetector([]).instantiate(null); + cd.hydrate(_DEFAULT_CONTEXT, null, null); cd.mode = CHECK_ONCE; cd.detectChanges(); @@ -793,6 +799,7 @@ export function main() { it("should not change the CHECK_ALWAYS", () => { var cd = createProtoChangeDetector([]).instantiate(null); + cd.hydrate(_DEFAULT_CONTEXT, null, null); cd.mode = CHECK_ALWAYS; cd.detectChanges(); @@ -809,7 +816,7 @@ export function main() { beforeEach(() => { var proto = createProtoChangeDetector([], [], [], null, ON_PUSH); checkedDetector = proto.instantiate(null); - checkedDetector.hydrate(null, null, null); + checkedDetector.hydrate(_DEFAULT_CONTEXT, null, null); checkedDetector.mode = CHECKED; // this directive is a component with ON_PUSH change detection @@ -829,7 +836,7 @@ export function main() { [dirRecordWithOnPush]); var cd = proto.instantiate(null); - cd.hydrate(null, null, directives); + cd.hydrate(_DEFAULT_CONTEXT, null, directives); expect(checkedDetector.mode).toEqual(CHECKED); @@ -898,6 +905,21 @@ export function main() { expect(pipe.destroyCalled).toBe(true); }); + + it("should throw when detectChanges is called on a dehydrated detector", () => { + var context = new Person('Bob'); + var c = createChangeDetector("propName", "name", context); + var cd = c["changeDetector"]; + var log = c["dispatcher"].log; + + cd.detectChanges(); + expect(log).toEqual(["propName=Bob"]); + + cd.dehydrate(); + var dehydratedException = new DehydratedException(); + expect(() => {cd.detectChanges()}).toThrowError(dehydratedException.toString()); + expect(log).toEqual(["propName=Bob"]); + }); }); describe("pipes", () => { @@ -1098,13 +1120,8 @@ class TestDirective { } class Person { - name: string; age: number; - address: Address; - constructor(name: string, address: Address = null) { - this.name = name; - this.address = address; - } + constructor(public name: string, public address: Address = null) {} sayHi(m) { return `Hi, ${m}`; } diff --git a/modules/angular2/test/transform/integration/two_annotations_files/expected/bar.ng_deps.dart b/modules/angular2/test/transform/integration/two_annotations_files/expected/bar.ng_deps.dart index 66f7e6878e..82f761a4df 100644 --- a/modules/angular2/test/transform/integration/two_annotations_files/expected/bar.ng_deps.dart +++ b/modules/angular2/test/transform/integration/two_annotations_files/expected/bar.ng_deps.dart @@ -34,6 +34,9 @@ class _MyComponent_ChangeDetector0 extends _gen.AbstractChangeDetector { : super(); void detectChangesInRecords(throwOnChange) { + if (!hydrated()) { + _gen.ChangeDetectionUtil.throwDehydrated(); + } var context = null; var change_context = false; var isChanged = false; @@ -56,7 +59,7 @@ class _MyComponent_ChangeDetector0 extends _gen.AbstractChangeDetector { _locals = null; } - hydrated() => _context == null; + hydrated() => _context != null; static _gen.ProtoChangeDetector newProtoChangeDetector( _gen.PipeRegistry registry, _gen.ChangeDetectorDefinition def) { diff --git a/modules/benchmarks/src/change_detection/change_detection_benchmark.ts b/modules/benchmarks/src/change_detection/change_detection_benchmark.ts index e45b47508a..269fce5a07 100644 --- a/modules/benchmarks/src/change_detection/change_detection_benchmark.ts +++ b/modules/benchmarks/src/change_detection/change_detection_benchmark.ts @@ -280,6 +280,7 @@ function setUpChangeDetection(changeDetection: ChangeDetection, iterations, obje new ChangeDetectorDefinition("proto", null, [], bindings, [directiveRecord])); var targetObj = new Obj(); + parentCd.hydrate(object, null, new FakeDirectives(targetObj)); for (var i = 0; i < iterations; ++i) { var cd = proto.instantiate(dispatcher); cd.hydrate(object, null, new FakeDirectives(targetObj));