Fix ChangeNotifier generic typing issue

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, []);
     });