Implement Model-Driven-Views spec for Dart

sdk/lib/observe/observable.dart

Index: sdk/lib/observe/observable.dart
diff --git a/sdk/lib/observe/observable.dart b/sdk/lib/observe/observable.dart
new file mode 100644
index 0000000000000000000000000000000000000000..b02a700e521c320bc873580a65348fd7282813c5
--- /dev/null
+++ b/sdk/lib/observe/observable.dart
@@ -0,0 +1,286 @@
+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+part of dart.observe;
+
+/**
+ * Converts the [Iterable] or [Map] to an [ObservableList] or [ObservableMap],
+ * respectively.
+ *
+ * If [value] is not one of those collection types, or is already [Observable],
+ * it will be returned unmodified.
+ *
+ * If [value] is a [Map], the resulting value will use the appropriate kind of
+ * backing map: either [HashMap], [LinkedHashMap], or [SplayTreeMap].
+ *
+ * By default this performs a deep conversion, but you can set [deep] to false
+ * for a shallow conversion. This does not handle circular data structures.
+ */
+// TODO(jmesserly): ObservableSet?
+toObservable(value, {bool deep: true}) =>

[floitsch, 2013/05/06 17:37:46]

This is non-extensible and doesn't really fit. It should not be part of the
public API of dart:observe.

[Jennifer Messerly, 2013/05/07 05:43:38]

Done -> moved to package:observe.

For what it's worth, the purpose of this: we got strong feedback that map and
list literals were too convenient, and using "new ObservableList.from" was too
verbose. That's why it is non-extensible--it only needs to support things that
Dart have Dart literals/JSON syntax. (I got rid of Set for this reason. I expect
some pushback from users though.)

+    deep ? _toObservableDeep(value) : _toObservableShallow(value);
+
+_toObserveShallow(value) {
+  if (value is Observable) return value;
+  if (value is Map) return new ObservableMap.from(value);
+  if (value is Iterable) return new ObservableList.from(value);
+  return value;
+}
+
+_toObservableDeep(value) {
+  if (value is Observable) return value;
+  if (value is Map) {
+    var result = new ObservableMap._createFromType(value);
+    value.forEach((k, v) {
+      result[_toObservableDeep(k)] = _toObservableDeep(v);
+    });
+    return result;
+  }
+  if (value is Iterable) {
+    return new ObservableList.from(value.map(_toObservableDeep));
+  }
+  return value;
+}
+
+
+/**
+ * Interface representing an observable object. This is used by data in
+ * model-view architectures to notify interested parties of [changes].
+ *
+ * This object does not require any specific technique to implement
+ * observability.
+ *
+ * You can use [ObservableMixin] as a base class or mixin to implement this.

[floitsch, 2013/05/06 17:37:46]

Don't make ObservableMixin a base class. Either provide ObservableBase or let
users write extends Object with ObservableMixin.

[Jennifer Messerly, 2013/05/07 05:43:38]

Done, added ObservableBase.

I'm curious though, can you explain more? I saw this in ListBase which is
defined as "Object extends ListMixin". I didn't understand why we'd want to
define two type names that do the same thing. Just aesthetics?

[floitsch, 2013/05/07 14:46:48]

They can have different semantics. A mixin might depend on another mixin. A base
class is, by definition self-contained.
For example:
  ObservableMixin
  ListMixin
  ObservableListMixin  // requires ObservableMixin and ListMixin

  ObservableListBase extends Object with ObservableListMixin, ListMixin,
ObservableMixin.

+ */
+abstract class Observable {
+  // TODO(jmesserly): should this be synchronous, and allow libraries to build
+  // the async batching?

[floitsch, 2013/05/06 17:37:46]

I would prefer if streams stay asynchronous. I just committed a change that
makes Future Completers send their values asynchronously. So things go rather in
that direction.

[Jennifer Messerly, 2013/05/07 05:43:38]

Ah, interesting. Would StreamController.add also be async eventually?

TODO removed, btw.

[floitsch, 2013/05/07 14:46:48]

Yes. This is planned.

+  /**
+   * The stream of change records to this object.
+   *
+   * Changes should be delivered in asynchronous batches by calling
+   * [queueChangeRecords].
+   * [deliverChangeRecords] can be called to force delivery.
+   */
+  Stream<List<ChangeRecord>> get changes;

[floitsch, 2013/05/06 17:37:46]

Why is this not just a Stream<ChangeRecord> ?

[Jennifer Messerly, 2013/05/07 05:43:38]

It's important to get a batch of changes, so you can efficiently compute deltas
and perform updates. They learned this lesson the hard way with DOM mutation
events. RafaelW (the guy driving MutationObservers and MDV) can probably tell
add more details.

(In fact, the JavaScript version is much more powerful. It allows a single
observer callback to get batches for multiple objects. ChangeSummary offers a
nice API for this: github.com/rafaelw/ChangeSummary)

[floitsch, 2013/05/07 14:46:48]

But wouldn't it be the task of a transformer to group these things? Maybe even
merge them (thus getting batches for multiple objects)?

[Jennifer Messerly, 2013/05/09 18:01:29]

Yeah, as long as such a thing can be built, that would be enough for me. You'd
need a way for the stream to receive all pending records in a single operation.

Actually--do streams support this already? It seems like something you might
want for I/O: 'give me all the bytes so far'

+
+  // TODO(jmesserly): remove these ASAP.
+  /**
+   * *Warning*: this method is temporary until dart2js supports mirrors.
+   * Gets the value of a field or index. This should return null if it was
+   * not found.
+   */
+  getValue(key);
+
+  /**
+   * *Warning*: this method is temporary until dart2js supports mirrors.
+   * Sets the value of a field or index. This should have no effect if the field
+   * was not found.
+   */
+  void setValue(key, Object value);
+}
+
+/**
+ * Mixin for implementing [Observable].
+ *
+ * When a field, property, or indexable item is changed, a derived class should
+ * call [notifyChange]. See that method for an example.
+ */
+abstract class ObservableMixin implements Observable {
+  // TODO(jmesserly): this has way too much overhead. We probably need our own
+  // stream.
+  StreamController<List<ChangeRecord>> _observers;
+  Stream<List<ChangeRecord>> _stream;
+  List<ChangeRecord> _changes;
+
+  Stream<List<ChangeRecord>> get changes {
+    if (_observers == null) {
+      _observers = new StreamController<List<ChangeRecord>>();
+      _stream = _observers.stream.asBroadcastStream();
+    }
+    return _stream;
+  }
+
+  void _deliverChanges() {
+    var changes = _changes;
+    _changes = null;
+    if (hasObservers && changes != null) {
+      // TODO(jmesserly): make "changes" immutable
+      _observers.add(changes);
+    }
+  }
+
+  /**
+   * True if this object has any observers, and should call [notifyChange] for
+   * changes.
+   */
+  bool get hasObservers => _observers != null && _observers.hasListener;
+
+  /**
+   * Notify that a [key] of this object has been changed.
+   *
+   * The key can also represent a field or indexed value of the object or list.
+   * The [kind] is one of the constants [ChangeRecord.INDEX],
+   * [ChangeRecord.FIELD], [ChangeRecord.INSERT], or [ChangeRecord.REMOVE].
+   *
+   * The [oldValue] and [newValue] are also recorded. If the change wasn't an
+   * insert or remove, and the two values are equal, no change will be recorded.
+   * For INSERT, oldValue should be null. For REMOVE, newValue should be null.
+   *
+   * For convenience this returns [newValue]. This makes it easy to use in a
+   * setter:
+   *
+   *     var _someField;
+   *     get someField => _someField;
+   *     set someField(value) {
+   *       _someField = notifyChange('someField', _someField, value);
+   *     }
+   */
+  Object notifyChange(key, Object oldValue, Object newValue,
+      {int kind: ChangeRecord.FIELD}) {
+
+    if (!hasObservers) return newValue;
+
+    // If this is an assignment (and not insert/remove) then check if
+    // the value actually changed. If not don't signal a change event.
+    // This helps programmers avoid some common cases of cycles in their code.
+    if ((kind & (ChangeRecord.INSERT | ChangeRecord.REMOVE)) == 0) {
+      if (oldValue == newValue) return newValue;

[Lasse Reichstein Nielsen, 2013/05/06 10:58:58]

Consider using "identical" instead. That won't make assumptions about the ==
operator on the values, and will catch all actual changes, not just changes to
unequal values.

[Jennifer Messerly, 2013/05/07 05:43:38]

Done.

[blois, 2013/05/07 16:04:49]

We should have a clear contract on what constitutes a value change- either at
this level or the path observer level, there is a good case for == behavior.

Using identical is probably correct for the return value, but the event may only
fire on !=?

I'm fine with identical for now, but have had to move away from strict identical
in the past for performance reasons.

+    }
+
+    if (_changes == null) {
+      _changes = [];
+      queueChangeRecords(_deliverChanges);
+    }
+    _changes.add(new ChangeRecord(key, oldValue, newValue, kind: kind));
+    return newValue;
+  }
+}
+
+/** Records a change to an [Observable]. */
+class ChangeRecord {

[floitsch, 2013/05/06 17:37:46]

I'm ok with Observable being a Stream of ChangeRecords. But this class is more
than just a ChangeRecord. It is a highly specialized version for properties.
This is a PropertyChangeRecord (or similar).

[Jennifer Messerly, 2013/05/07 05:43:38]

Agreed. I will try and split it:

* ObjectChangeRecord
* MapChangeRecord
* ListChangeRecord

Objects -- has the field name as Symbol
Map -- has the key, plus insert/remove/update
List -- has the list change delta, which is what we want anyway

It might take some surgery; do you have strong preference if this happens before
or after initial checkin? It might be easier to see/work with in a follow up CL.

[floitsch, 2013/05/07 14:46:48]

fine with later. I don't like the "ObjectChangeRecord" name, though. Everything
is an Object in Dart. FieldChangeRecord?

[Jennifer Messerly, 2013/05/09 18:01:29]

Funny, I had the same thought last night after I went home. FieldChangeRecord
sounds great.

update: on further reflection I renamed "field" to "property" terminology.

+  // Note: the target object is omitted because it makes it difficult
+  // to proxy change records if you're using an observable kind to aid
+  // your implementation.
+  // However: if we allow one observer to get batched changes for multiple
+  // objects, we'll need to add target.
+
+  // Note: kind values were chosen for easy masking in the observable expression
+  // implementation. However in [kind] it will only have one value.
+
+  // TODO(jmesserly): is there any value in keeping FIELD and INDEX distinct?
+  /** [kind] denoting set of a field. */
+  static const FIELD = 1;
+
+  /** [kind] denoting an in-place update event using `[]=`. */
+  static const INDEX = 2;
+
+  /**
+   * [kind] denoting an insertion into a list. Insertions prepend in front of
+   * the given index, so insert at 0 means an insertion at the beginning of the
+   * list. The index will be provided in [key].
+   */
+  static const INSERT = INDEX | 4;
+
+  /** [kind] denoting a remove from a list. */
+  static const REMOVE = INDEX | 8;
+
+  /** Whether the change was a [FIELD], [INDEX], [INSERT], or [REMOVE]. */
+  final int kind;
+
+  // TODO(jmesserly): for fields, is key a String or Symbol? Right now it's a
+  // String.
+  /**
+   * The key that changed. The value depends on the [kind] of change:
+   *
+   * - [FIELD]: the field name that was set.
+   * - [INDEX], [INSERT], and [REMOVE]: the index or key that was changed.
+   *   This will be an integer for [ObservableList] but can be anything for
+   *   [ObservableMap].
+   */
+  final key;
+
+  /** The previous value of the member. */

[Lasse Reichstein Nielsen, 2013/05/06 10:58:58]

Will this be null for an insert?

[Jennifer Messerly, 2013/05/07 05:43:38]

That's what I had thought. It turns out for a List, you need the original value
so you can reconstruct the previous list state.

Per Florian's comment above, I'll try restructuring this so:
* only MapChangeRecord cares about insert/remove
* ListChangeRecord contains change records in the form we want
* ObjectChangeRecord has no notion of insert/remove

+  final oldValue;
+
+  /** The new value of the member. */
+  final newValue;

[Lasse Reichstein Nielsen, 2013/05/06 10:58:58]

Will this be null for a remove?

[Jennifer Messerly, 2013/05/07 05:43:38]

yes

+
+  ChangeRecord(this.key, this.oldValue, this.newValue,
+      {this.kind: ChangeRecord.FIELD});
+
+  // Note: these two methods are here mainly to make testing easier.
+  bool operator ==(other) {
+    return other is ChangeRecord && kind == other.kind && key == other.key &&

[Lasse Reichstein Nielsen, 2013/05/06 10:58:58]

Consider using identical for the tests. At least the value tests. The key will
likely be using == as equivalence in a Map.

[Jennifer Messerly, 2013/05/07 05:43:38]

I removed == and hashCode

+        oldValue == other.oldValue && newValue == other.newValue;
+  }
+
+  int get hashCode => _hash4(kind, key, oldValue, newValue);
+
+  String toString() {
+    // TODO(jmesserly): const map would be nice here, but it must be string
+    // literal :(
+    String typeStr;
+    switch (kind) {
+      case FIELD: typeStr = 'field'; break;
+      case INDEX: typeStr = 'index'; break;
+      case INSERT: typeStr = 'insert'; break;
+      case REMOVE: typeStr = 'remove'; break;
+    }
+    return '#<ChangeRecord $typeStr $key from $oldValue to $newValue>';
+  }
+}
+
+// TODO(jmesserly): helpers to combine hash codes. Reuse these from somewhere.
+int _hash2(x, y) => x.hashCode * 31 + y.hashCode;
+
+int _hash3(x, y, z) => _hash2(_hash2(x, y), z);

[Lasse Reichstein Nielsen, 2013/05/06 10:58:58]

Unused?

[Jennifer Messerly, 2013/05/07 05:43:38]

Done.

+
+int _hash4(w, x, y, z) => _hash2(_hash2(w, x), _hash2(y, z));

[Lasse Reichstein Nielsen, 2013/05/06 10:58:58]

This gives x and y the same multiplier (31), resulting in
  w * 31^2 + (x+y) * 31 + z
so it's symmetric in x and y which might give worse results. 
At least it's used so that x is a key and y is a value, which are likely to have
different types.

[floitsch, 2013/05/06 17:37:46]

_hash2(w, _hash2(x, _hash2(y, z))) should solve this without more overhead.
correct?

[Jennifer Messerly, 2013/05/07 05:43:38]

removed hash helpers :)
it would be great to have these in the standard libs/pkgs though. *nudge* :)

[Lasse Reichstein Nielsen, 2013/05/07 07:35:45]

It doesn't parallelize as well, but
 _hash2(_hash2(w, x) * 31, _hash2(y, z));
would give the same result.

+
+
+/**
+ * Synchronously deliver [Observable.changes] for all observables.
+ * If new changes are added as a result of delivery, this will keep running
+ * until all pending change records are delivered.
+ */
+// TODO(jmesserly): this is a bit different from the ES Harmony version, which
+// allows delivery of changes to a particular observer:
+// http://wiki.ecmascript.org/doku.php?id=harmony:observe#object.deliverchangerecords
+// However the binding system needs delivery of everything, along the lines of:
+// https://github.com/toolkitchen/mdv/blob/stable/src/model.js#L19
+// https://github.com/rafaelw/ChangeSummary/blob/master/change_summary.js#L590
+// TODO(jmesserly): in the future, we can use this to trigger dirty checking.
+void deliverChangeRecords() {

[floitsch, 2013/05/06 17:37:46]

This looks completely wrong.

Why is there global state for Observables? Buffer the stream if that's
necessary, but don't put all callbacks into a global list.

[Jennifer Messerly, 2013/05/07 05:43:38]

It does to me too :)
The capability is needed, but I hate having global state.
we discussed this offline, but basically:

I need a way to get a timeslice *after* all observers have propagated. This is
critical for testing, and for applications to access their completed DOM before
it renders.

It sounds like runAsync will queue something before the browser renders, but it
isn't clear how to get a timeslice that happens after all observers have
finished. I'm open to ideas on how to solve this.

[floitsch, 2013/05/07 14:46:48]

I would rather add support for adding something to the end of the instant. Since
it doesn't exist yet, keep it, but keep reminding us.

+  if (_deliverCallbacks == null) return;
+
+  while (_deliverCallbacks.length > 0) {
+    var deliverCallbacks = _deliverCallbacks;

[Lasse Reichstein Nielsen, 2013/05/06 10:58:58]

Consider using a Queue instead to avoid doing the bookkeeping yourself. It's
doing exactly what you want (allow adding and removing in LIFO order) and it's
efficient too.

[Jennifer Messerly, 2013/05/07 05:43:38]

Done.

+    // Use empty list so [queueChangeRecords] don't reschedule this method.
+    _deliverCallbacks = [];
+
+    for (var deliver in deliverCallbacks) {
+      try {
+        deliver();
+      } catch (e, s) {
+        // Schedule the error to be top-leveled later.
+        new Completer().completeError(e, s);
+      }
+    }
+  }
+
+  // Use null list so [queueChangeRecords] will reschedule this method.
+  _deliverCallbacks = null;
+}
+
+/** Queues an action to happen during the [deliverChangeRecords] timeslice. */
+void queueChangeRecords(void deliverChanges()) {
+  if (_deliverCallbacks == null) {
+    _deliverCallbacks = [];
+    runAsync(deliverChangeRecords);
+  }
+  _deliverCallbacks.add(deliverChanges);
+}
+
+List<Function> _deliverCallbacks;