UIP-2629 getDartComponent warning for ReactElements

lib/src/util/dom_util.dart

@@ -128,7 +128,7 @@ void setSelectionRange(/* TextInputElement | TextAreaElement */Element input, in
   if (input is TextAreaElement) {
     input.setSelectionRange(start, end, direction);
   } else if (input is InputElement && supportsSelectionRange(input)) {
-    if (browser.isChrome) {
+    if (browser.isChrome || browser.isFirefox) {
       final inputType = input.getAttribute('type');
 
       if (inputType == 'email' || inputType == 'number') {
@@ -161,7 +161,7 @@ int getSelectionStart(Element input) {
   } else if (input is TextInputElement && supportsSelectionRange(input)) {
     final inputType = input.getAttribute('type');
 
-    if (browser.isChrome) {
+    if (browser.isChrome || browser.isFirefox) {
       if (inputType == 'email' || inputType == 'number') {
         return null;
       }

lib/src/util/react_wrappers.dart

@@ -280,6 +280,40 @@ react.Component getDartComponent(/* [1] */ instance) {
     return null;
   }
 
+  assert(() {
+    if (isValidElement(instance)) {
+      // `print` instead of `ValidationUtil.warn` so that this message shows up
+      // in the test output when running `ddev test`.
+      print(unindent(
+          '''
+          * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * 
+          * WARNING: `getDartComponent`                                                                                                       
+          *                                                                                            
+          * react-dart 4.0 will no longer support retrieving Dart components from                                                                                           
+          * `ReactElement` (pre-mounted VDOM instances) in order to prevent memory leaks.                                                                                            
+          *                                                                                            
+          * In react-dart 4.0, this call to `getDartComponent` will return `null`.                                                                                           
+          *                                                                                            
+          * Please switch over to using the mounted JS component instance.                                                                                           
+          *                                                                                            
+          * Example:                                                                                           
+          *     // Before                                                                                           
+          *     var vdom = Button()('Click me');                                                                                           
+          *     react_dom.render(vdom, mountNode);                                                                                           
+          *     var dartInstance = getDartComponent(vdom);                                                                                           
+          *                                                                                                
+          *     // Fixed                                                                                           
+          *     var vdom = Button()('Click me');                                                                                           
+          *     var jsInstance = react_dom.render(vdom, mountNode);                                                                                           
+          *     var dartInstance = getDartComponent(jsInstance);                            
+          *                                                                                 
+          * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+          '''
+      ));
+    }
+    return true;
+  });
+
   return _getInternal(instance)?.component;
 }
 

test/over_react/util/dom_util_test.dart

@@ -214,7 +214,11 @@ main() {
             // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360
             test(type, () {
               sharedInputSetSelectionRangeTest(type);
-            }, testOn: 'js && !chrome');
+
+            // Tests run in `ddev coverage` don't respect tags and show up as the 'vm' platform
+            // so we can use this to disable certain browser tests during coverage.
+            // Workaround for https://github.com/Workiva/dart_dev/issues/200
+            }, testOn: '!(blink || firefox || vm)');
           } else {
             test(type, () { sharedInputSetSelectionRangeTest(type); });
           }
@@ -233,7 +237,7 @@ main() {
       });
 
       // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360
-      group('without throwing an error in Google Chrome when `props.type` is', () {
+      group('without throwing an error in Google Chrome (or Dartium/content_shell) or Firefox when `props.type` is', () {
         void verifyLackOfException() {
           expect(renderedInstance, isNotNull, reason: 'test setup sanity check');
           expect(inputElement, isNotNull, reason: 'test setup sanity check');
@@ -266,7 +270,7 @@ main() {
           inputElement = findDomNode(renderedInstance);
           verifyLackOfException();
         });
-      }, testOn: 'chrome');
+      }, testOn: 'blink || firefox');
     });
   });
 
@@ -301,23 +305,33 @@ main() {
       });
 
       group('for an `<input>` of type:', () {
-        void sharedInputGetSelectionStartTest(String type) {
+        void sharedInputGetSelectionStartTest(String type, {bool shouldReturnNull: false}) {
           renderedInstance = renderAttachedToDocument((Dom.input()
             ..defaultValue = testValue
             ..type = type
           )());
           inputElement = findDomNode(renderedInstance);
+          setSelectionRange(inputElement, testValue.length, testValue.length);
+
           var selectionStart = getSelectionStart(inputElement);
 
-          expect(selectionStart, testValue.length);
+          expect(selectionStart, shouldReturnNull ? isNull : testValue.length);
         }
 
         for (var type in inputTypesWithSelectionRangeSupport) {
           if (type == 'email' || type == 'number') {
             // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360
+            test('$type, returning null in Google Chrome (or Dartium/content_shell) or Firefox', () {
+              sharedInputGetSelectionStartTest(type, shouldReturnNull: true);
+            }, testOn: 'blink || firefox');
+
             test(type, () {
-              sharedInputGetSelectionStartTest(type);
-            }, testOn: 'js && !chrome');
+              sharedInputGetSelectionStartTest(type, shouldReturnNull: false);
+
+            // Tests run in `ddev coverage` don't respect tags and show up as the 'vm' platform
+            // so we can use this to disable certain browser tests during coverage.
+            // Workaround for https://github.com/Workiva/dart_dev/issues/200
+            }, testOn: '!(blink || firefox || vm)');
           } else {
             test(type, () { sharedInputGetSelectionStartTest(type); });
           }
@@ -337,7 +351,7 @@ main() {
       });
 
       // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360
-      group('by returning null and not throwing an error in Google Chrome when `props.type` is', () {
+      group('by returning null and not throwing an error in Google Chrome (or Dartium/content_shell) or Firefox when `props.type` is', () {
         void verifyReturnsNullWithoutException() {
           expect(renderedInstance, isNotNull, reason: 'test setup sanity check');
           expect(inputElement, isNotNull, reason: 'test setup sanity check');
@@ -366,7 +380,7 @@ main() {
           inputElement = findDomNode(renderedInstance);
           verifyReturnsNullWithoutException();
         });
-      }, testOn: 'chrome');
+      }, testOn: 'blink || firefox');
     });
   });
 }

test/over_react/util/react_wrappers_test.dart

@@ -15,6 +15,7 @@
 @JS()
 library react_wrappers_test;
 
+import 'dart:async';
 import 'dart:html';
 
 import 'package:js/js.dart';
@@ -439,6 +440,30 @@ main() {
         var renderedInstance = render(Dom.div());
         expect(getDartComponent(renderedInstance), isNull);
       });
+
+      group('', () {
+        final messageMatcher = contains('react-dart 4.0 will no longer support retrieving Dart components from');
+
+        test('warns when passed a ReactElement', () {
+          ReactElement instance = Wrapper()();
+          expect(() => getDartComponent(instance), prints(messageMatcher));
+        }, testOn: 'dart-vm');
+
+        test('does not when passed a ReactElement in JS', () {
+          ReactElement instance = Wrapper()();
+          expect(() => getDartComponent(instance), isNot(prints(messageMatcher)));
+        }, testOn: 'js', tags: 'no-ddc');
+
+        test('does not warn when passed a ReactComponent', () {
+          var renderedInstance = render(Wrapper());
+          expect(() => getDartComponent(renderedInstance), isNot(prints(messageMatcher)));
+        });
+
+        test('does not warn when passed a DOM component', () {
+          var renderedInstance = render(Dom.div());
+          expect(() => getDartComponent(renderedInstance), isNot(prints(messageMatcher)));
+        });
+      });
     });
 
     group('getProps', () {