Fix ChangeNotifier generic typing issue (#76)

Fix ChangeNotifier generic typing issue - ChangeRecord.ANY and ChangeRecord.None is not typesafe for any subclasses of ChangeNotifier that subclass the generic.

Proposed solution is to output a `ChangeRecords extends List<ChangeRecord>` with additional metadata to indicate the change is ANY or NONE.

Advantage of this change is that it is backwards compatible with existing code while fixing type exceptions for future code.

CHANGELOG.md

@@ -1,3 +1,10 @@
+## 0.22.1+5
+
+Fix generic type error that occurs when using ChangeNotifier with a subclass of ChangeRecord.
+Previously, calling `notifyChanges()` on `class Foo with ChangeNotifier<CustomChangeRecord> {}`
+would throw a type error. Now, the `changes` stream emits a custom `ChangeRecords` class that
+implements the `List` interface. This change is backwards compatible.
+
 ## 0.22.1+4
 
 * Support Dart 2 stable.

lib/observable.dart

@@ -6,9 +6,14 @@ library observable;
 
 export 'src/change_notifier.dart' show ChangeNotifier, PropertyChangeNotifier;
 export 'src/differs.dart' show Differ, EqualityDiffer, ListDiffer, MapDiffer;
-export 'src/records.dart'
-    show ChangeRecord, ListChangeRecord, MapChangeRecord, PropertyChangeRecord;
 export 'src/observable.dart';
 export 'src/observable_list.dart';
 export 'src/observable_map.dart';
+export 'src/records.dart'
+    show
+        ChangeRecord,
+        ChangeRecords,
+        ListChangeRecord,
+        MapChangeRecord,
+        PropertyChangeRecord;
 export 'src/to_observable.dart';

lib/src/change_notifier.dart

@@ -51,18 +51,16 @@ class ChangeNotifier<C extends ChangeRecord> implements Observable<C> {
   @override
   @mustCallSuper
   bool deliverChanges() {
-    List<ChangeRecord> changes;
     if (_scheduled && hasObservers) {
-      if (_queue != null) {
-        changes = freezeInDevMode(_queue);
-        _queue = null;
-      } else {
-        changes = ChangeRecord.ANY;
-      }
+      final changes = _queue == null
+          ? ChangeRecords<C>.any()
+          : ChangeRecords.wrap(freezeInDevMode(_queue));
+      _queue = null;
       _scheduled = false;
       _changes.add(changes);
+      return true;
     }
-    return changes != null;
+    return false;
   }
 
   /// Whether [changes] has at least one active listener.

lib/src/records.dart

@@ -19,10 +19,71 @@ class ChangeRecord {
   ///
   /// May be used to produce lower-GC-pressure records where more verbose change
   /// records will not be used directly.
-  static const List<ChangeRecord> ANY = const [const ChangeRecord()];
+  static const ANY = ChangeRecords<ChangeRecord>.any();
 
   /// Signifies no changes occurred.
-  static const List<ChangeRecord> NONE = const [];
+  static const NONE = ChangeRecords<ChangeRecord>.none();
 
   const ChangeRecord();
 }
+
+/// Represents a list of change records.
+///
+/// The motivation for implementing the list interface is to fix a typing
+/// issue with ChangeRecord.ANY while maintaining backwards compatibility with
+/// existing code.
+class ChangeRecords<RecordType extends ChangeRecord>
+    extends DelegatingList<RecordType> {
+  // This is a covariant unfortunately because generics cannot be used in a
+  // const constructor. Should be sound however since the equality check does
+  // not do any mutations.
+  static const _listEquals = ListEquality<ChangeRecord>();
+
+  final bool _isAny;
+
+  final List<RecordType> _delegate;
+
+  /// Represents any change where the list of changes is irrelevant.
+  const ChangeRecords.any() : this._(const [], true);
+
+  /// Represents a null change where nothing happened.
+  const ChangeRecords.none() : this._(const [], false);
+
+  /// Wraps around a list of records.
+  ///
+  /// Note: this wraps around a shallow copy of [list]. If [list] is modified,
+  /// then it is modified within this change record as well. This is provide a
+  /// const constructor for [ChangeRecords].
+  const ChangeRecords.wrap(List<RecordType> list) : this._(list, false);
+
+  /// Creates a change record list from a deep copy of [it].
+  ChangeRecords.fromIterable(Iterable<RecordType> it)
+      : this._(List.unmodifiable(it), false);
+
+  const ChangeRecords._(this._delegate, this._isAny) : super(_delegate);
+
+  @override
+  int get hashCode => hash2(_delegate, _isAny);
+
+  /// Equal if this and [other] have the same generic type and either both are
+  /// any records or both are not any records and have the same list of entries.
+  ///
+  /// E.g.
+  ///   ChangeRecords<CR1>.any() == ChangeRecords<CR1>.any()
+  ///   ChangeRecords<CR1>.any() != ChangeRecords<CR2>.any()
+  ///
+  /// List of records checked with deep comparison.
+  @override
+  bool operator ==(Object other) =>
+      identical(this, other) ||
+      other is ChangeRecords &&
+          runtimeType == other.runtimeType &&
+          ((_isAny && other._isAny) ||
+              (!_isAny &&
+                  !other._isAny &&
+                  _listEquals.equals(_delegate, other._delegate)));
+
+  @override
+  String toString() =>
+      _isAny ? 'ChangeRecords.any' : 'ChangeRecords($_delegate)';
+}

test/change_notifier_test.dart

@@ -0,0 +1,100 @@
+import 'dart:async';
+
+import 'package:observable/observable.dart';
+import 'package:test/test.dart';
+
+import 'observable_test_utils.dart';
+
+void main() {
+  group(ChangeRecords, () {
+    test('any changes', () {
+      expectChanges(const ChangeRecords<A>.any(), const ChangeRecords<A>.any());
+      expectChanges(ChangeRecords<A>.any(), ChangeRecords<A>.any());
+      expectNotChanges(ChangeRecords<A>.any(), ChangeRecords<A>.wrap([]));
+      expectNotChanges(ChangeRecords<A>.any(), ChangeRecords<B>.any());
+      expectNotChanges(ChangeRecords<B>.any(), ChangeRecords<C>.any());
+    });
+
+    test('some changes', () {
+      expectChanges(ChangeRecords<A>.fromIterable([A()]),
+          ChangeRecords<A>.fromIterable([A()]));
+      expectChanges(ChangeRecords<A>.fromIterable([B(1), B(2)]),
+          ChangeRecords<A>.fromIterable([B(1), B(2)]));
+      expectNotChanges(ChangeRecords<A>.fromIterable([A()]),
+          ChangeRecords<A>.fromIterable([A(), A()]));
+      expectNotChanges(ChangeRecords<B>.fromIterable([B(1)]),
+          ChangeRecords<A>.fromIterable([B(2)]));
+      expectNotChanges(ChangeRecords<B>.fromIterable([B(1)]),
+          ChangeRecords<A>.fromIterable([C()]));
+    });
+  });
+
+  group(ChangeNotifier, () {
+    Future<void> runTest<T extends ChangeRecord>(
+        FutureOr<void> runFn(ChangeNotifier<T> cn),
+        FutureOr<void> testFn(ChangeRecords<T> cr)) async {
+      final cn = ChangeNotifier<T>();
+
+      cn.changes.listen((value) {
+        expect(value, TypeMatcher<ChangeRecords<T>>());
+        testFn(value);
+      });
+
+      await runFn(cn);
+
+      return Future(() {});
+    }
+
+    test(
+        'delivers any record when no change notified',
+        () => runTest<A>((cn) {
+              cn.notifyChange();
+            }, (cr) {
+              expectChanges(cr, ChangeRecords<A>.any());
+            }));
+
+    test(
+        'delivers expectChangesed changes',
+        () => runTest<B>((cn) {
+              cn..notifyChange(B(1))..notifyChange(B(2))..notifyChange(B(3));
+            }, (cr) {
+              expectChanges(cr, ChangeRecords<B>.wrap([B(1), B(2), B(3)]));
+            }));
+  });
+}
+
+class A extends ChangeRecord {
+  @override
+  bool operator ==(Object other) =>
+      identical(this, other) || other is A && runtimeType == other.runtimeType;
+
+  @override
+  int get hashCode => 0;
+}
+
+class B extends A {
+  final int value;
+
+  B(this.value);
+
+  @override
+  bool operator ==(Object other) =>
+      identical(this, other) ||
+      super == other &&
+          other is B &&
+          runtimeType == other.runtimeType &&
+          this.value == other.value;
+
+  @override
+  int get hashCode => value.hashCode;
+}
+
+class C extends A {
+  @override
+  bool operator ==(Object other) =>
+      identical(this, other) ||
+      super == other && other is C && runtimeType == other.runtimeType;
+
+  @override
+  int get hashCode => 2;
+}

test/list_change_test.dart

@@ -15,7 +15,7 @@ import 'observable_test_utils.dart';
 main() => listChangeTests();
 
 // TODO(jmesserly): port or write array fuzzer tests
-listChangeTests() {
+void listChangeTests() {
   StreamSubscription sub;
   var model;
 
@@ -24,43 +24,45 @@ listChangeTests() {
     model = null;
   });
 
-  _delta(i, r, a) => new ListChangeRecord(model, i, removed: r, addedCount: a);
+  ListChangeRecord<E> _delta<E>(int i, List<E> r, int a,
+          {ObservableList<E> typedModel}) =>
+      ListChangeRecord(typedModel ?? model, i, removed: r, addedCount: a);
 
   test('sequential adds', () {
-    model = toObservable([]);
+    final model = ObservableList();
     model.add(0);
 
-    var summary;
+    List<ListChangeRecord> summary;
     sub = model.listChanges.listen((r) => summary = r);
 
     model.add(1);
     model.add(2);
 
     expect(summary, null);
     return new Future(() {
-      expectChanges(summary, [_delta(1, [], 2)]);
+      expect(summary, [_delta(1, [], 2, typedModel: model)]);
       expect(summary[0].added, [1, 2]);
       expect(summary[0].removed, []);
     });
   });
 
   test('List Splice Truncate And Expand With Length', () {
-    model = toObservable(['a', 'b', 'c', 'd', 'e']);
+    final model = ObservableList<String>.from(['a', 'b', 'c', 'd', 'e']);
 
-    var summary;
+    List<ListChangeRecord<String>> summary;
     sub = model.listChanges.listen((r) => summary = r);
 
     model.length = 2;
     return new Future(() {
-      expectChanges(summary, [
-        _delta(2, ['c', 'd', 'e'], 0)
+      expect(summary, [
+        _delta(2, ['c', 'd', 'e'], 0, typedModel: model)
       ]);
       expect(summary[0].added, []);
       expect(summary[0].removed, ['c', 'd', 'e']);
       summary = null;
       model.length = 5;
     }).then(newMicrotask).then((_) {
-      expectChanges(summary, [_delta(2, [], 3)]);
+      expect(summary, [_delta(2, [], 3, typedModel: model)]);
       expect(summary[0].added, [null, null, null]);
       expect(summary[0].removed, []);
     });