X Tutup
Skip to content

Commit f60fa14

Browse files
tboschvsavkin
tbosch
authored and
vsavkin
committed
feat(core): drop ChangeDetectionStrategy.OnPushObserve
BREAKING CHANGE: `OnPushObserve` was an experimental feature for Dart and had conceptual performance problems, as setting up observables is slow. Use `OnPush` instead.
1 parent d900f5c commit f60fa14

23 files changed

+14
-1019
lines changed

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

Lines changed: 0 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {BindingTarget} from './binding_record';
1616
import {Locals} from './parser/locals';
1717
import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
1818
import {wtfCreateScope, wtfLeave, WtfScopeFn} from '../profile/profile';
19-
import {isObservable} from './observable_facade';
2019
import {ObservableWrapper} from 'angular2/src/facade/async';
2120

2221
var _scope_check: WtfScopeFn = wtfCreateScope(`ChangeDetector#check(ascii id, bool throwOnChange)`);
@@ -42,10 +41,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
4241
propertyBindingIndex: number;
4342
outputSubscriptions: any[];
4443

45-
// This is an experimental feature. Works only in Dart.
46-
subscriptions: any[];
47-
streams: any[];
48-
4944
dispatcher: ChangeDispatcher;
5045

5146

@@ -158,10 +153,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
158153
this.mode = ChangeDetectionUtil.changeDetectionMode(this.strategy);
159154
this.context = context;
160155

161-
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
162-
this.observeComponent(context);
163-
}
164-
165156
this.locals = locals;
166157
this.pipes = pipes;
167158
this.hydrateDirectives(dispatcher);
@@ -176,11 +167,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
176167
dehydrate(): void {
177168
this.dehydrateDirectives(true);
178169

179-
// This is an experimental feature. Works only in Dart.
180-
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
181-
this._unsubsribeFromObservables();
182-
}
183-
184170
this._unsubscribeFromOutputs();
185171

186172
this.dispatcher = null;
@@ -248,19 +234,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
248234
}
249235
}
250236

251-
// This is an experimental feature. Works only in Dart.
252-
private _unsubsribeFromObservables(): void {
253-
if (isPresent(this.subscriptions)) {
254-
for (var i = 0; i < this.subscriptions.length; ++i) {
255-
var s = this.subscriptions[i];
256-
if (isPresent(this.subscriptions[i])) {
257-
s.cancel();
258-
this.subscriptions[i] = null;
259-
}
260-
}
261-
}
262-
}
263-
264237
private _unsubscribeFromOutputs(): void {
265238
if (isPresent(this.outputSubscriptions)) {
266239
for (var i = 0; i < this.outputSubscriptions.length; ++i) {
@@ -270,53 +243,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
270243
}
271244
}
272245

273-
// This is an experimental feature. Works only in Dart.
274-
observeValue(value: any, index: number): any {
275-
if (isObservable(value)) {
276-
this._createArrayToStoreObservables();
277-
if (isBlank(this.subscriptions[index])) {
278-
this.streams[index] = value.changes;
279-
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
280-
} else if (this.streams[index] !== value.changes) {
281-
this.subscriptions[index].cancel();
282-
this.streams[index] = value.changes;
283-
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
284-
}
285-
}
286-
return value;
287-
}
288-
289-
// This is an experimental feature. Works only in Dart.
290-
observeDirective(value: any, index: number): any {
291-
if (isObservable(value)) {
292-
this._createArrayToStoreObservables();
293-
var arrayIndex = this.numberOfPropertyProtoRecords + index + 2; // +1 is component
294-
this.streams[arrayIndex] = value.changes;
295-
this.subscriptions[arrayIndex] = value.changes.listen((_) => this.ref.markForCheck());
296-
}
297-
return value;
298-
}
299-
300-
// This is an experimental feature. Works only in Dart.
301-
observeComponent(value: any): any {
302-
if (isObservable(value)) {
303-
this._createArrayToStoreObservables();
304-
var index = this.numberOfPropertyProtoRecords + 1;
305-
this.streams[index] = value.changes;
306-
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
307-
}
308-
return value;
309-
}
310-
311-
private _createArrayToStoreObservables(): void {
312-
if (isBlank(this.subscriptions)) {
313-
this.subscriptions = ListWrapper.createFixedSize(this.numberOfPropertyProtoRecords +
314-
this.directiveIndices.length + 2);
315-
this.streams = ListWrapper.createFixedSize(this.numberOfPropertyProtoRecords +
316-
this.directiveIndices.length + 2);
317-
}
318-
}
319-
320246
getDirectiveFor(directives: any, index: number): any {
321247
return directives.getDirectiveFor(this.directiveIndices[index]);
322248
}

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,8 @@ export class ChangeDetectorJITGenerator {
5757
this.directiveRecords = definition.directiveRecords;
5858
this._names = new CodegenNameUtil(this.records, this.eventBindings, this.directiveRecords,
5959
this.changeDetectionUtilVarName);
60-
this._logic =
61-
new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
62-
this.changeDetectorStateVarName, this.changeDetectionStrategy);
60+
this._logic = new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
61+
this.changeDetectorStateVarName);
6362
this.typeName = sanitizeName(`ChangeDetector_${this.id}`);
6463
}
6564

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

Lines changed: 9 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,14 @@ import {codify, combineGeneratedStrings, rawString} from './codegen_facade';
44
import {ProtoRecord, RecordType} from './proto_record';
55
import {BindingTarget} from './binding_record';
66
import {DirectiveRecord} from './directive_record';
7-
import {ChangeDetectionStrategy} from './constants';
87
import {BaseException} from 'angular2/src/facade/exceptions';
98

109
/**
1110
* Class responsible for providing change detection logic for change detector classes.
1211
*/
1312
export class CodegenLogicUtil {
1413
constructor(private _names: CodegenNameUtil, private _utilName: string,
15-
private _changeDetectorStateName: string,
16-
private _changeDetection: ChangeDetectionStrategy) {}
14+
private _changeDetectorStateName: string) {}
1715

1816
/**
1917
* Generates a statement which updates the local variable representing `protoRec` with the current
@@ -51,31 +49,29 @@ export class CodegenLogicUtil {
5149
break;
5250

5351
case RecordType.PropertyRead:
54-
rhs = this._observe(`${context}.${protoRec.name}`, protoRec);
52+
rhs = `${context}.${protoRec.name}`;
5553
break;
5654

5755
case RecordType.SafeProperty:
58-
var read = this._observe(`${context}.${protoRec.name}`, protoRec);
59-
rhs =
60-
`${this._utilName}.isValueBlank(${context}) ? null : ${this._observe(read, protoRec)}`;
56+
var read = `${context}.${protoRec.name}`;
57+
rhs = `${this._utilName}.isValueBlank(${context}) ? null : ${read}`;
6158
break;
6259

6360
case RecordType.PropertyWrite:
6461
rhs = `${context}.${protoRec.name} = ${getLocalName(protoRec.args[0])}`;
6562
break;
6663

6764
case RecordType.Local:
68-
rhs = this._observe(`${localsAccessor}.get(${rawString(protoRec.name)})`, protoRec);
65+
rhs = `${localsAccessor}.get(${rawString(protoRec.name)})`;
6966
break;
7067

7168
case RecordType.InvokeMethod:
72-
rhs = this._observe(`${context}.${protoRec.name}(${argString})`, protoRec);
69+
rhs = `${context}.${protoRec.name}(${argString})`;
7370
break;
7471

7572
case RecordType.SafeMethodInvoke:
7673
var invoke = `${context}.${protoRec.name}(${argString})`;
77-
rhs =
78-
`${this._utilName}.isValueBlank(${context}) ? null : ${this._observe(invoke, protoRec)}`;
74+
rhs = `${this._utilName}.isValueBlank(${context}) ? null : ${invoke}`;
7975
break;
8076

8177
case RecordType.InvokeClosure:
@@ -95,7 +91,7 @@ export class CodegenLogicUtil {
9591
break;
9692

9793
case RecordType.KeyedRead:
98-
rhs = this._observe(`${context}[${getLocalName(protoRec.args[0])}]`, protoRec);
94+
rhs = `${context}[${getLocalName(protoRec.args[0])}]`;
9995
break;
10096

10197
case RecordType.KeyedWrite:
@@ -112,16 +108,6 @@ export class CodegenLogicUtil {
112108
return `${getLocalName(protoRec.selfIndex)} = ${rhs};`;
113109
}
114110

115-
/** @internal */
116-
_observe(exp: string, rec: ProtoRecord): string {
117-
// This is an experimental feature. Works only in Dart.
118-
if (this._changeDetection === ChangeDetectionStrategy.OnPushObserve) {
119-
return `this.observeValue(${exp}, ${rec.selfIndex})`;
120-
} else {
121-
return exp;
122-
}
123-
}
124-
125111
genPropertyBindingTargets(propertyBindingTargets: BindingTarget[],
126112
genDebugInfo: boolean): string {
127113
var bs = propertyBindingTargets.map(b => {
@@ -202,15 +188,7 @@ export class CodegenLogicUtil {
202188
}
203189
}
204190

205-
private _genReadDirective(index: number) {
206-
var directiveExpr = `this.getDirectiveFor(directives, ${index})`;
207-
// This is an experimental feature. Works only in Dart.
208-
if (this._changeDetection === ChangeDetectionStrategy.OnPushObserve) {
209-
return `this.observeDirective(${directiveExpr}, ${index})`;
210-
} else {
211-
return directiveExpr;
212-
}
213-
}
191+
private _genReadDirective(index: number) { return `this.getDirectiveFor(directives, ${index})`; }
214192

215193
genHydrateDetectors(directiveRecords: DirectiveRecord[]): string {
216194
var res = [];

‎modules/angular2/src/core/change_detection/constants.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,6 @@ export enum ChangeDetectionStrategy {
6262
* `Default` means that the change detector's mode will be set to `CheckAlways` during hydration.
6363
*/
6464
Default,
65-
66-
/**
67-
* This is an experimental feature. Works only in Dart.
68-
*/
69-
OnPushObserve
7065
}
7166

7267
/**
@@ -78,8 +73,7 @@ export var CHANGE_DETECTION_STRATEGY_VALUES = [
7873
ChangeDetectionStrategy.CheckAlways,
7974
ChangeDetectionStrategy.Detached,
8075
ChangeDetectionStrategy.OnPush,
81-
ChangeDetectionStrategy.Default,
82-
ChangeDetectionStrategy.OnPushObserve
76+
ChangeDetectionStrategy.Default
8377
];
8478

8579
/**

‎modules/angular2/src/core/change_detection/dynamic_change_detector.ts‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -110,12 +110,6 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
110110
this.values[0] = this.context;
111111
this.dispatcher = dispatcher;
112112

113-
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
114-
for (var i = 0; i < this.directiveIndices.length; ++i) {
115-
var index = this.directiveIndices[i];
116-
super.observeDirective(this._getDirectiveFor(index), i);
117-
}
118-
}
119113
this.outputSubscriptions = [];
120114
for (var i = 0; i < this._directiveRecords.length; ++i) {
121115
var r = this._directiveRecords[i];
@@ -300,9 +294,6 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
300294
}
301295

302296
var currValue = this._calculateCurrValue(proto, values, locals);
303-
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
304-
super.observeValue(currValue, proto.selfIndex);
305-
}
306297

307298
if (proto.shouldBeChecked()) {
308299
var prevValue = this._readSelf(proto, values);

‎modules/angular2/src/core/change_detection/observable_facade.dart‎

Lines changed: 0 additions & 5 deletions
This file was deleted.

‎modules/angular2/src/core/change_detection/observable_facade.ts‎

Lines changed: 0 additions & 3 deletions
This file was deleted.

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

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -106,21 +106,6 @@ export function getDefinition(id: string): TestDefinition {
106106
[_DirectiveUpdating.basicRecords[0], _DirectiveUpdating.basicRecords[1]], genConfig);
107107
testDef = new TestDefinition(id, cdDef, null);
108108

109-
} else if (id == "onPushObserveBinding") {
110-
var records = _createBindingRecords("a");
111-
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], records,
112-
[], [], genConfig);
113-
testDef = new TestDefinition(id, cdDef, null);
114-
115-
} else if (id == "onPushObserveComponent") {
116-
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], [], [],
117-
[], genConfig);
118-
testDef = new TestDefinition(id, cdDef, null);
119-
120-
} else if (id == "onPushObserveDirective") {
121-
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], [], [],
122-
[_DirectiveUpdating.recordNoCallbacks], genConfig);
123-
testDef = new TestDefinition(id, cdDef, null);
124109
} else if (id == "updateElementProduction") {
125110
var genConfig = new ChangeDetectorGenConfig(false, false, true);
126111
var records = _createBindingRecords("name");
@@ -151,12 +136,7 @@ export function getAllDefinitions(): TestDefinition[] {
151136
allDefs = allDefs.concat(StringMapWrapper.keys(_DirectiveUpdating.availableDefinitions));
152137
allDefs = allDefs.concat(_availableEventDefinitions);
153138
allDefs = allDefs.concat(_availableHostEventDefinitions);
154-
allDefs = allDefs.concat([
155-
"onPushObserveBinding",
156-
"onPushObserveComponent",
157-
"onPushObserveDirective",
158-
"updateElementProduction"
159-
]);
139+
allDefs = allDefs.concat(["updateElementProduction"]);
160140
return allDefs.map(getDefinition);
161141
}
162142

0 commit comments

Comments
 (0)
X Tutup