Add check for JS refs, beef up test coverage to test all ref types with all factory types

Author: greglittlefield-wf

lib/react_client/react_interop.dart

@@ -82,8 +82,10 @@ class Ref<T> {
   T get current {
     final jsCurrent = jsRef.current;
 
-    if (jsCurrent is! Element) {
-      final dartCurrent = (jsCurrent as ReactComponent)?.dartComponent;
+    // Note: this ReactComponent check will pass for many types of JS objects,
+    // so don't assume for sure that it's a ReactComponent
+    if (jsCurrent is! Element && jsCurrent is ReactComponent) {
+      final dartCurrent = jsCurrent.dartComponent;
 
       if (dartCurrent != null) {
         return dartCurrent as T;

lib/src/react_client/chain_refs.dart

@@ -1,3 +1,4 @@
+import 'package:js/js_util.dart';
 import 'package:react/react_client/react_interop.dart';
 
 /// Returns a ref that updates both [ref1] and [ref2], effectively
@@ -29,14 +30,14 @@ dynamic chainRefs(dynamic ref1, dynamic ref2) {
 
 /// Like [chainRefs], but takes in a list of [refs].
 dynamic chainRefList(List<dynamic> refs) {
+  final nonNullRefs = refs.where((ref) => ref != null).toList(growable: false);
+
   // Wrap in an assert so iteration doesn't take place unnecessarily
   assert(() {
-    refs.forEach(_validateChainRefsArg);
+    nonNullRefs.forEach(_validateChainRefsArg);
     return true;
   }());
 
-  final nonNullRefs = refs.where((ref) => ref != null).toList();
-
   // Return null if there are no refs to chain
   if (nonNullRefs.isEmpty) return null;
 
@@ -82,6 +83,15 @@ dynamic chainRefList(List<dynamic> refs) {
 }
 
 void _validateChainRefsArg(dynamic ref) {
-  assert(ref is! Function || ref is Function(Null), 'callback refs must take a single argument');
-  assert(ref is! String, 'String refs cannot be chained');
+  if (ref is Function(Null) ||
+      ref is Ref ||
+      // Need to duck-type since `is JsRef` will return true for most JS objects.
+      (ref is JsRef && hasProperty(ref, 'current'))) {
+    return;
+  }
+
+  if (ref is String) throw AssertionError('String refs cannot be chained');
+  if (ref is Function) throw AssertionError('callback refs must take a single argument');
+
+  throw AssertionError('Invalid ref type: $ref');
 }

test/factory/common_factory_tests.dart

@@ -332,101 +332,26 @@ void refTests<T>(ReactComponentFactoryProxy factory, {void verifyRefValue(dynami
     });
   });
 
-  group('chainRefs works', () {
-    test('with two typed callback refs', () {
-      var calls = <Map>[];
-
-      rtu.renderIntoDocument(factory({
-        'ref': chainRefs(
-          (T ref) => calls.add({'name': 'ref1', 'value': ref}),
-          (T ref) => calls.add({'name': 'ref2', 'value': ref}),
-        ),
-      }));
-
-      expect(calls, [
-        {'name': 'ref1', 'value': anything},
-        {'name': 'ref2', 'value': anything},
-      ]);
-      verifyRefValue(calls[0]['value']);
-      verifyRefValue(calls[1]['value']);
-    });
-
-    test('with a typed and untyped callback refs', () {
-      var calls = <Map>[];
-
-      rtu.renderIntoDocument(factory({
-        'ref': chainRefs(
-          (T ref) => calls.add({'name': 'ref1', 'value': ref}),
-          (ref) => calls.add({'name': 'ref2', 'value': ref}),
-        ),
-      }));
-
-      expect(calls, [
-        {'name': 'ref1', 'value': anything},
-        {'name': 'ref2', 'value': anything},
-      ]);
-      verifyRefValue(calls[0]['value']);
-      verifyRefValue(calls[1]['value']);
-    });
-
-    test('with two createRef refs', () {
-      final ref1 = react.createRef<T>();
-      final ref2 = react.createRef<T>();
-
-      rtu.renderIntoDocument(factory({
-        'ref': chainRefs(ref1, ref2),
-      }));
-
-      verifyRefValue(ref1.current);
-      verifyRefValue(ref2.current);
-    });
-
-    test('with a typed callback ref and a createRef', () {
-      var callbackRefCallValues = [];
-      final refObject = react.createRef<T>();
-
-      rtu.renderIntoDocument(factory({
-        'ref': chainRefs(
-          (T ref) => callbackRefCallValues.add(ref),
-          refObject,
-        ),
-      }));
-
-      expect(callbackRefCallValues, hasLength(1));
-      verifyRefValue(callbackRefCallValues[0]);
-      verifyRefValue(refObject.current);
-    });
-
-    // Other cases tested in chainRefs's own tests
-  });
-
   group('chainRefList works', () {
-    test('with a list of refs, ignoring any null values', () {
-      var calls = <Map>[];
+    test('with all different types of values, ignoring null', () {
+      final testCases = RefTestCase.allChainable<T>();
 
+      T refValue;
       rtu.renderIntoDocument(factory({
         'ref': chainRefList([
+          (ref) => refValue = ref,
           null,
-          (ref) => calls.add({'name': 'ref1', 'value': ref}),
           null,
-          (ref) => calls.add({'name': 'ref2', 'value': ref}),
-          (ref) => calls.add({'name': 'ref3', 'value': ref}),
-          null,
-          null,
-          (ref) => calls.add({'name': 'ref4', 'value': ref}),
+          ...testCases.map((t) => t.ref),
         ]),
       }));
+      // Test setup check: verify refValue is correct,
+      // which we'll use below to verify refs were updated.
+      verifyRefValue(refValue);
 
-      expect(calls, [
-        {'name': 'ref1', 'value': anything},
-        {'name': 'ref2', 'value': anything},
-        {'name': 'ref3', 'value': anything},
-        {'name': 'ref4', 'value': anything},
-      ]);
-      verifyRefValue(calls[0]['value']);
-      verifyRefValue(calls[1]['value']);
-      verifyRefValue(calls[2]['value']);
-      verifyRefValue(calls[3]['value']);
+      for (final testCase in testCases) {
+        testCase.verifyRefWasUpdated(refValue);
+      }
     });
 
     // Other cases tested in chainRefList's own tests

test/react_client/chain_refs_test.dart

@@ -1,15 +1,20 @@
 @TestOn('browser')
+@JS()
 library react.chain_refs_test;
 
+import 'package:js/js.dart';
+import 'package:react/react_client.dart';
 import 'package:react/src/react_client/chain_refs.dart';
 import 'package:test/test.dart';
 
+import '../util.dart';
+
 main() {
   group('Ref chaining utils:', () {
     testRef(_) {}
 
     group('chainRefs', () {
-      // Actual chaining is tested in refTests so it can be tested functionally with each component type.
+      // Chaining and arg validation is tested via chainRefList.
 
       group('skips chaining and passes through arguments when', () {
         test('both arguments are null', () {
@@ -24,12 +29,10 @@ main() {
           expect(chainRefs(testRef, null), same(testRef));
         });
       });
-
-      // Arg validation is covered by `chainRefList` tests
     });
 
     group('chainRefList', () {
-      // Actual chaining is tested in refTests so it can be tested functionally with each component type.
+      // Chaining is tested functionally in refTests with each component type.
 
       group('skips chaining and passes through arguments when', () {
         test('the list is empty', () {
@@ -49,15 +52,44 @@ main() {
         });
       });
 
-      group('asserts that inputs arg are not', () {
+      group('raises an assertion when inputs are invalid:', () {
         test('strings', () {
           expect(() => chainRefList([testRef, 'bad ref']), throwsA(isA<AssertionError>()));
         });
 
         test('unsupported function types', () {
           expect(() => chainRefList([testRef, () {}]), throwsA(isA<AssertionError>()));
         });
+
+        test('other objects', () {
+          expect(() => chainRefList([testRef, Object()]), throwsA(isA<AssertionError>()));
+        });
+
+        // test JS interop objects since type-checking anonymous interop objects
+        test('non-createRef anonymous JS interop objects', () {
+          expect(() => chainRefList([testRef, JsTypeAnonymous()]), throwsA(isA<AssertionError>()));
+        });
+
+        // test JS interop objects since type-checking anonymous interop objects
+        test('non-createRef JS interop objects', () {
+          expect(() => chainRefList([testRef, JsType()]), throwsA(isA<AssertionError>()));
+        });
       }, tags: 'no-dart2js');
+
+      test('does not raise an assertion for valid input refs', () {
+        expect(() => chainRefList(RefTestCase.allChainable().map((r) => r.ref).toList()), returnsNormally);
+      });
     });
   });
 }
+
+@JS()
+@anonymous
+class JsTypeAnonymous {
+  external factory JsTypeAnonymous();
+}
+
+@JS()
+class JsType {
+  external JsType();
+}

test/react_client/chain_refs_test.html

@@ -6,6 +6,10 @@
     <script src="packages/react/react_with_addons.js"></script>
     <script src="packages/react/react_dom.js"></script>
 
+    <script>
+        function JsType() {}
+    </script>
+
     <link rel="x-dart-test" href="chain_refs_test.dart">
     <script src="packages/test/dart.js"></script>
 </head>

test/util.dart

@@ -6,11 +6,12 @@ library react.test.util;
 import 'dart:js_util' show getProperty;
 
 import 'package:js/js.dart';
-import 'package:react/react_client/react_interop.dart';
-
-import 'package:react/react_test_utils.dart' as rtu;
+import 'package:meta/meta.dart';
 import 'package:react/react.dart' as react;
 import 'package:react/react_client/js_backed_map.dart';
+import 'package:react/react_client/react_interop.dart';
+import 'package:react/react_test_utils.dart' as rtu;
+import 'package:test/test.dart';
 
 @JS('Object.keys')
 external List _objectKeys(obj);
@@ -54,3 +55,57 @@ Map unmodifiableMap([Map map1, Map map2, Map map3, Map map4]) {
   if (map4 != null) merged.addAll(map4);
   return new Map.unmodifiable(merged);
 }
+
+/// A test case that can be used for consuming a specific kind of ref and verifying
+/// it was updated properly when rendered.
+class RefTestCase {
+  final dynamic ref;
+  final Function(dynamic actualValue) verifyRefWasUpdated;
+
+  RefTestCase._({@required this.ref, @required this.verifyRefWasUpdated});
+
+  static RefTestCase untypedCallbackRefCase() {
+    final untypedCallbackRefCalls = [];
+    return RefTestCase._(
+      ref: (value) => untypedCallbackRefCalls.add(value),
+      verifyRefWasUpdated: (actualValue) => expect(untypedCallbackRefCalls, [same(actualValue)]),
+    );
+  }
+
+  static RefTestCase typedCallbackRefCase<T>() {
+    final typedCallbackRefCalls = [];
+    return RefTestCase._(
+      ref: (T value) => typedCallbackRefCalls.add(value),
+      verifyRefWasUpdated: (actualValue) => expect(typedCallbackRefCalls, [same(actualValue)]),
+    );
+  }
+
+  static RefTestCase refObjectCase<T>() {
+    final refObjectRef = createRef<T>();
+    return RefTestCase._(
+      ref: refObjectRef,
+      verifyRefWasUpdated: (actualValue) => expect(refObjectRef.current, same(actualValue)),
+    );
+  }
+
+  static RefTestCase jsRefObjectCase<T>() {
+    final jsRefObjectRef = React.createRef();
+    return RefTestCase._(
+      ref: jsRefObjectRef,
+      verifyRefWasUpdated: (actualValue) => expect(jsRefObjectRef.current, same(actualValue)),
+    );
+  }
+
+  /// Test cases for each of the valid, chainable ref types:
+  ///
+  /// 1. callback ref with untyped argument
+  /// 2. callback ref with typed argument
+  /// 3. createRef (Dart wrapper)
+  /// 4. createRef (JS object)
+  static List<RefTestCase> allChainable<T>() => [
+        untypedCallbackRefCase(),
+        typedCallbackRefCase<T>(),
+        refObjectCase<T>(),
+        jsRefObjectCase<T>(),
+      ];
+}