cargdev/angular
1
0
Fork 0
Code Issues 12 Pull Requests Actions Packages Projects Releases Wiki Activity

feat(core): drop ChangeDetectionStrategy.OnPushObserve

Browse Source 
BREAKING CHANGE:

`OnPushObserve` was an experimental
feature for Dart and had
conceptual performance problems,
as setting up observables is slow.
Use `OnPush` instead.
This commit is contained in:
Tobias Bosch
2016-02-16 14:36:34 -08:00
committed by vsavkin
parent d900f5c075
commit f60fa14767
23 changed files with 14 additions and 1019 deletions
Download Patch File Download Diff File Expand all files Collapse all files

74
modules/angular2/src/core/change_detection/abstract_change_detector.ts
View File

@ -16,7 +16,6 @@ import {BindingTarget} from './binding_record';
import {Locals} from './parser/locals';
import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
import {wtfCreateScope, wtfLeave, WtfScopeFn} from '../profile/profile';
import {isObservable} from './observable_facade';
import {ObservableWrapper} from 'angular2/src/facade/async';
var _scope_check: WtfScopeFn = wtfCreateScope(`ChangeDetector#check(ascii id, bool throwOnChange)`);
@ -42,10 +41,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
propertyBindingIndex: number;
outputSubscriptions: any[];
// This is an experimental feature. Works only in Dart.
subscriptions: any[];
streams: any[];
dispatcher: ChangeDispatcher;
@ -158,10 +153,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
this.mode = ChangeDetectionUtil.changeDetectionMode(this.strategy);
this.context = context;
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
this.observeComponent(context);
}
this.locals = locals;
this.pipes = pipes;
this.hydrateDirectives(dispatcher);
@ -176,11 +167,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
dehydrate(): void {
this.dehydrateDirectives(true);
// This is an experimental feature. Works only in Dart.
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
this._unsubsribeFromObservables();
}
this._unsubscribeFromOutputs();
this.dispatcher = null;
@ -248,19 +234,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
}
}
// This is an experimental feature. Works only in Dart.
private _unsubsribeFromObservables(): void {
if (isPresent(this.subscriptions)) {
for (var i = 0; i < this.subscriptions.length; ++i) {
var s = this.subscriptions[i];
if (isPresent(this.subscriptions[i])) {
s.cancel();
this.subscriptions[i] = null;
}
}
}
}
private _unsubscribeFromOutputs(): void {
if (isPresent(this.outputSubscriptions)) {
for (var i = 0; i < this.outputSubscriptions.length; ++i) {
@ -270,53 +243,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
}
}
// This is an experimental feature. Works only in Dart.
observeValue(value: any, index: number): any {
if (isObservable(value)) {
this._createArrayToStoreObservables();
if (isBlank(this.subscriptions[index])) {
this.streams[index] = value.changes;
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
} else if (this.streams[index] !== value.changes) {
this.subscriptions[index].cancel();
this.streams[index] = value.changes;
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
}
}
return value;
}
// This is an experimental feature. Works only in Dart.
observeDirective(value: any, index: number): any {
if (isObservable(value)) {
this._createArrayToStoreObservables();
var arrayIndex = this.numberOfPropertyProtoRecords + index + 2; // +1 is component
this.streams[arrayIndex] = value.changes;
this.subscriptions[arrayIndex] = value.changes.listen((_) => this.ref.markForCheck());
}
return value;
}
// This is an experimental feature. Works only in Dart.
observeComponent(value: any): any {
if (isObservable(value)) {
this._createArrayToStoreObservables();
var index = this.numberOfPropertyProtoRecords + 1;
this.streams[index] = value.changes;
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
}
return value;
}
private _createArrayToStoreObservables(): void {
if (isBlank(this.subscriptions)) {
this.subscriptions = ListWrapper.createFixedSize(this.numberOfPropertyProtoRecords +
this.directiveIndices.length + 2);
this.streams = ListWrapper.createFixedSize(this.numberOfPropertyProtoRecords +
this.directiveIndices.length + 2);
}
}
getDirectiveFor(directives: any, index: number): any {
return directives.getDirectiveFor(this.directiveIndices[index]);
}

5
modules/angular2/src/core/change_detection/change_detection_jit_generator.ts
View File

@ -57,9 +57,8 @@ export class ChangeDetectorJITGenerator {
this.directiveRecords = definition.directiveRecords;
this._names = new CodegenNameUtil(this.records, this.eventBindings, this.directiveRecords,
this.changeDetectionUtilVarName);
this._logic =
new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
this.changeDetectorStateVarName, this.changeDetectionStrategy);
this._logic = new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
this.changeDetectorStateVarName);
this.typeName = sanitizeName(`ChangeDetector_${this.id}`);
}

40
modules/angular2/src/core/change_detection/codegen_logic_util.ts
View File

@ -4,7 +4,6 @@ import {codify, combineGeneratedStrings, rawString} from './codegen_facade';
import {ProtoRecord, RecordType} from './proto_record';
import {BindingTarget} from './binding_record';
import {DirectiveRecord} from './directive_record';
import {ChangeDetectionStrategy} from './constants';
import {BaseException} from 'angular2/src/facade/exceptions';
/**
@ -12,8 +11,7 @@ import {BaseException} from 'angular2/src/facade/exceptions';
*/
export class CodegenLogicUtil {
constructor(private _names: CodegenNameUtil, private _utilName: string,
private _changeDetectorStateName: string,
private _changeDetection: ChangeDetectionStrategy) {}
private _changeDetectorStateName: string) {}
/**
* Generates a statement which updates the local variable representing `protoRec` with the current
@ -51,13 +49,12 @@ export class CodegenLogicUtil {
break;
case RecordType.PropertyRead:
rhs = this._observe(`${context}.${protoRec.name}`, protoRec);
rhs = `${context}.${protoRec.name}`;
break;
case RecordType.SafeProperty:
var read = this._observe(`${context}.${protoRec.name}`, protoRec);
rhs =
`${this._utilName}.isValueBlank(${context}) ? null : ${this._observe(read, protoRec)}`;
var read = `${context}.${protoRec.name}`;
rhs = `${this._utilName}.isValueBlank(${context}) ? null : ${read}`;
break;
case RecordType.PropertyWrite:
@ -65,17 +62,16 @@ export class CodegenLogicUtil {
break;
case RecordType.Local:
rhs = this._observe(`${localsAccessor}.get(${rawString(protoRec.name)})`, protoRec);
rhs = `${localsAccessor}.get(${rawString(protoRec.name)})`;
break;
case RecordType.InvokeMethod:
rhs = this._observe(`${context}.${protoRec.name}(${argString})`, protoRec);
rhs = `${context}.${protoRec.name}(${argString})`;
break;
case RecordType.SafeMethodInvoke:
var invoke = `${context}.${protoRec.name}(${argString})`;
rhs =
`${this._utilName}.isValueBlank(${context}) ? null : ${this._observe(invoke, protoRec)}`;
rhs = `${this._utilName}.isValueBlank(${context}) ? null : ${invoke}`;
break;
case RecordType.InvokeClosure:
@ -95,7 +91,7 @@ export class CodegenLogicUtil {
break;
case RecordType.KeyedRead:
rhs = this._observe(`${context}[${getLocalName(protoRec.args[0])}]`, protoRec);
rhs = `${context}[${getLocalName(protoRec.args[0])}]`;
break;
case RecordType.KeyedWrite:
@ -112,16 +108,6 @@ export class CodegenLogicUtil {
return `${getLocalName(protoRec.selfIndex)} = ${rhs};`;
}
/** @internal */
_observe(exp: string, rec: ProtoRecord): string {
// This is an experimental feature. Works only in Dart.
if (this._changeDetection === ChangeDetectionStrategy.OnPushObserve) {
return `this.observeValue(${exp}, ${rec.selfIndex})`;
} else {
return exp;
}
}
genPropertyBindingTargets(propertyBindingTargets: BindingTarget[],
genDebugInfo: boolean): string {
var bs = propertyBindingTargets.map(b => {
@ -202,15 +188,7 @@ export class CodegenLogicUtil {
}
}
private _genReadDirective(index: number) {
var directiveExpr = `this.getDirectiveFor(directives, ${index})`;
// This is an experimental feature. Works only in Dart.
if (this._changeDetection === ChangeDetectionStrategy.OnPushObserve) {
return `this.observeDirective(${directiveExpr}, ${index})`;
} else {
return directiveExpr;
}
}
private _genReadDirective(index: number) { return `this.getDirectiveFor(directives, ${index})`; }
genHydrateDetectors(directiveRecords: DirectiveRecord[]): string {
var res = [];

8
modules/angular2/src/core/change_detection/constants.ts
View File

@ -62,11 +62,6 @@ export enum ChangeDetectionStrategy {
* `Default` means that the change detector's mode will be set to `CheckAlways` during hydration.
*/
Default,
/**
* This is an experimental feature. Works only in Dart.
*/
OnPushObserve
}
/**
@ -78,8 +73,7 @@ export var CHANGE_DETECTION_STRATEGY_VALUES = [
ChangeDetectionStrategy.CheckAlways,
ChangeDetectionStrategy.Detached,
ChangeDetectionStrategy.OnPush,
ChangeDetectionStrategy.Default,
ChangeDetectionStrategy.OnPushObserve
ChangeDetectionStrategy.Default
];
/**

9
modules/angular2/src/core/change_detection/dynamic_change_detector.ts
View File

@ -110,12 +110,6 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
this.values[0] = this.context;
this.dispatcher = dispatcher;
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
for (var i = 0; i < this.directiveIndices.length; ++i) {
var index = this.directiveIndices[i];
super.observeDirective(this._getDirectiveFor(index), i);
}
}
this.outputSubscriptions = [];
for (var i = 0; i < this._directiveRecords.length; ++i) {
var r = this._directiveRecords[i];
@ -300,9 +294,6 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
}
var currValue = this._calculateCurrValue(proto, values, locals);
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
super.observeValue(currValue, proto.selfIndex);
}
if (proto.shouldBeChecked()) {
var prevValue = this._readSelf(proto, values);

5
modules/angular2/src/core/change_detection/observable_facade.dart
View File

@ -1,5 +0,0 @@
library change_detection.observable_facade;
import 'package:observe/observe.dart';
bool isObservable(value) => value is Observable;

3
modules/angular2/src/core/change_detection/observable_facade.ts
View File

@ -1,3 +0,0 @@
export function isObservable(value: any): boolean {
return false;
}

22
modules/angular2/test/core/change_detection/change_detector_config.ts
View File

@ -106,21 +106,6 @@ export function getDefinition(id: string): TestDefinition {
[_DirectiveUpdating.basicRecords[0], _DirectiveUpdating.basicRecords[1]], genConfig);
testDef = new TestDefinition(id, cdDef, null);
} else if (id == "onPushObserveBinding") {
var records = _createBindingRecords("a");
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], records,
[], [], genConfig);
testDef = new TestDefinition(id, cdDef, null);
} else if (id == "onPushObserveComponent") {
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], [], [],
[], genConfig);
testDef = new TestDefinition(id, cdDef, null);
} else if (id == "onPushObserveDirective") {
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], [], [],
[_DirectiveUpdating.recordNoCallbacks], genConfig);
testDef = new TestDefinition(id, cdDef, null);
} else if (id == "updateElementProduction") {
var genConfig = new ChangeDetectorGenConfig(false, false, true);
var records = _createBindingRecords("name");
@ -151,12 +136,7 @@ export function getAllDefinitions(): TestDefinition[] {
allDefs = allDefs.concat(StringMapWrapper.keys(_DirectiveUpdating.availableDefinitions));
allDefs = allDefs.concat(_availableEventDefinitions);
allDefs = allDefs.concat(_availableHostEventDefinitions);
allDefs = allDefs.concat([
"onPushObserveBinding",
"onPushObserveComponent",
"onPushObserveDirective",
"updateElementProduction"
]);
allDefs = allDefs.concat(["updateElementProduction"]);
return allDefs.map(getDefinition);
}

97
modules/angular2/test/core/change_detection/change_detector_spec.ts
View File

@ -51,7 +51,6 @@ import {JitProtoChangeDetector} from 'angular2/src/core/change_detection/jit_pro
import {OnDestroy} from 'angular2/src/core/linker/interfaces';
import {getDefinition} from './change_detector_config';
import {createObservableModel} from './change_detector_spec_util';
import {getFactoryById} from './generated/change_detector_classes';
import {IS_DART} from 'angular2/src/facade/lang';
import {EventEmitter, ObservableWrapper} from 'angular2/src/facade/async';
@ -1105,102 +1104,6 @@ export function main() {
expect(childDirectiveDetectorOnPush.mode).toEqual(ChangeDetectionStrategy.CheckOnce);
});
if (IS_DART) {
describe('OnPushObserve', () => {
it('should mark OnPushObserve detectors as CheckOnce when an observable fires an event',
fakeAsync(() => {
var context = new TestDirective();
context.a = createObservableModel();
var cd = _createWithoutHydrate('onPushObserveBinding').changeDetector;
cd.hydrate(context, null, directives, null);
cd.detectChanges();
expect(cd.mode).toEqual(ChangeDetectionStrategy.Checked);
context.a.pushUpdate();
tick();
expect(cd.mode).toEqual(ChangeDetectionStrategy.CheckOnce);
}));
it('should mark OnPushObserve detectors as CheckOnce when an observable context fires an event',
fakeAsync(() => {
var context = createObservableModel();
var cd = _createWithoutHydrate('onPushObserveComponent').changeDetector;
cd.hydrate(context, null, directives, null);
cd.detectChanges();
expect(cd.mode).toEqual(ChangeDetectionStrategy.Checked);
context.pushUpdate();
tick();
expect(cd.mode).toEqual(ChangeDetectionStrategy.CheckOnce);
}));
it('should mark OnPushObserve detectors as CheckOnce when an observable directive fires an event',
fakeAsync(() => {
var dir = createObservableModel();
var directives = new TestDispatcher([dir], []);
var cd = _createWithoutHydrate('onPushObserveDirective').changeDetector;
cd.hydrate(_DEFAULT_CONTEXT, null, directives, null);
cd.detectChanges();
expect(cd.mode).toEqual(ChangeDetectionStrategy.Checked);
dir.pushUpdate();
tick();
expect(cd.mode).toEqual(ChangeDetectionStrategy.CheckOnce);
}));
it('should unsubscribe from an old observable when an object changes',
fakeAsync(() => {
var originalModel = createObservableModel();
var context = new TestDirective();
context.a = originalModel;
var cd = _createWithoutHydrate('onPushObserveBinding').changeDetector;
cd.hydrate(context, null, directives, null);
cd.detectChanges();
context.a = createObservableModel();
cd.mode = ChangeDetectionStrategy.CheckOnce;
cd.detectChanges();
// Updating this model will not reenable the detector. This model is not longer
// used.
originalModel.pushUpdate();
tick();
expect(cd.mode).toEqual(ChangeDetectionStrategy.Checked);
}));
it('should unsubscribe from observables when dehydrating', fakeAsync(() => {
var originalModel = createObservableModel();
var context = new TestDirective();
context.a = originalModel;
var cd = _createWithoutHydrate('onPushObserveBinding').changeDetector;
cd.hydrate(context, null, directives, null);
cd.detectChanges();
cd.dehydrate();
context.a = "not an observable model";
cd.hydrate(context, null, directives, null);
cd.detectChanges();
// Updating this model will not reenable the detector. This model is not longer
// used.
originalModel.pushUpdate();
tick();
expect(cd.mode).toEqual(ChangeDetectionStrategy.Checked);
}));
});
}
});
});

27
modules/angular2/test/core/change_detection/change_detector_spec_util.dart
View File

@ -1,27 +0,0 @@
library angular.change_detection.change_detector_spec_util;
import 'package:observe/observe.dart' show Observable;
import 'dart:async';
dynamic createObservableModel() {
return new Entity();
}
class Entity implements Observable {
Stream changes;
StreamController controller;
Entity() {
controller = new StreamController.broadcast();
changes = controller.stream;
}
pushUpdate() {
controller.add("new");
}
bool get hasObservers => null;
bool deliverChanges() => null;
notifyPropertyChange(Symbol field, Object oldValue, Object newValue) => null;
void notifyChange(record) {}
}

3
modules/angular2/test/core/change_detection/change_detector_spec_util.ts
View File

@ -1,3 +0,0 @@
export function createObservableModel(): any {
return null;
}

1
modules/angular2/test/public_api_spec.ts
View File

@ -802,7 +802,6 @@ var NG_CORE = [
'ChangeDetectionStrategy#Default',
'ChangeDetectionStrategy#Detached',
'ChangeDetectionStrategy#OnPush',
'ChangeDetectionStrategy#OnPushObserve',
'ChangeDetectionStrategy#values',
'ChangeDetectionStrategy',
'ChangeDetectionStrategy.index',