Merge pull request #382 from Workiva/fix-react-dom-typings

rmconsole5-wk · web-flow · commit c59ba121308d · 2023-11-17T11:39:45.000-06:00
FED-1910 Fix react_dom API typings, add tests
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,22 @@
+## 7.0.1
+
+Breaking change - fix nullability/typings for `ReactDom.findDomNode` and `ReactDom.render` from `package:react/react_client/react_interop.dart`:
+```diff
+-Element findDOMNode(dynamic object);
++Element? findDOMNode(dynamic object);
+
+-ReactComponent render(ReactElement component, Element element);
++dynamic render(dynamic component, Element element);
+```
+
+The previous typings were incorrect:
+- `findDOMNode` returns null in many cases, but its return type was incorrectly non-nullable.
+- `render` returns null for some cases (function components, `null`), `Element` for DOM components, and `CharacterData` for strings and numbers, but was incorrectly typed as non-nullable `ReactComponent`. The `component` argument also accepts `null` and other "ReactNode" arguments to rendered, but its type is incorrectly non-nullable and restricted to just `ReactElement`.
+
+These typings only affect these APIs under the `ReactDom` class in package:react/react_client/react_interop.dart, and not the top-level `Function`-typed `findDOMNode` and `render` APIs exported from `package:react/react_dom.dart`, which most consumers use.
+
+Because these typings were incorrect and will lead to runtime errors in some cases, and the changes have a low likelihood of causing breakages, we feel it's appropriate to release these changes as a patch. 
+
 ## 7.0.0
 
 - Migrate to null safety
diff --git a/lib/react_client/react_interop.dart b/lib/react_client/react_interop.dart
@@ -274,8 +274,8 @@ ReactComponentFactoryProxy memo2(ReactComponentFactoryProxy factory,
 }
 
 abstract class ReactDom {
-  static Element findDOMNode(object) => ReactDOM.findDOMNode(object);
-  static ReactComponent render(ReactElement component, Element element) => ReactDOM.render(component, element);
+  static Element? findDOMNode(dynamic object) => ReactDOM.findDOMNode(object);
+  static dynamic render(dynamic component, Element element) => ReactDOM.render(component, element);
   static bool unmountComponentAtNode(Element element) => ReactDOM.unmountComponentAtNode(element);
 
   /// Returns a a portal that renders [children] into a [container].
diff --git a/lib/src/react_client/dart2_interop_workaround_bindings.dart b/lib/src/react_client/dart2_interop_workaround_bindings.dart
@@ -8,8 +8,8 @@ import 'package:react/react_client/react_interop.dart';
 
 @JS()
 abstract class ReactDOM {
-  external static Element findDOMNode(object);
-  external static ReactComponent render(ReactElement component, Element element);
+  external static Element? findDOMNode(dynamic object);
+  external static dynamic render(dynamic component, Element element);
   external static bool unmountComponentAtNode(Element element);
   external static ReactPortal createPortal(dynamic children, Element container);
 }
diff --git a/test/lifecycle_test.dart b/test/lifecycle_test.dart
@@ -480,7 +480,7 @@ main() {
         final mountNode = DivElement();
         final renderedInstance = react_dom.render(components2.SetStateTest({}), mountNode);
         final component = getDartComponent<LifecycleTestHelper>(renderedInstance);
-        final renderedNode = findDomNode(renderedInstance);
+        final renderedNode = findDomNode(renderedInstance)!;
         LifecycleTestHelper.staticLifecycleCalls.clear();
         component.setState({'shouldThrow': true});
 
@@ -1518,7 +1518,7 @@ void sharedLifecycleTests<T extends react.Component>({
       test('when shouldComponentUpdate returns false', () {
         final mountNode = DivElement();
         final renderedInstance = react_dom.render(SetStateTest({'shouldUpdate': false}), mountNode);
-        final renderedNode = findDomNode(renderedInstance);
+        final renderedNode = findDomNode(renderedInstance)!;
         final component = getDartComponent<LifecycleTestHelper>(renderedInstance);
 
         react_test_utils.Simulate.click(renderedNode.children.first);
@@ -1541,7 +1541,7 @@ void sharedLifecycleTests<T extends react.Component>({
       test('when shouldComponentUpdate returns true', () {
         final mountNode = DivElement();
         final renderedInstance = react_dom.render(SetStateTest({}), mountNode);
-        final renderedNode = findDomNode(renderedInstance);
+        final renderedNode = findDomNode(renderedInstance)!;
         final component = getDartComponent<LifecycleTestHelper>(renderedInstance);
 
         react_test_utils.Simulate.click(renderedNode.children.first);
diff --git a/test/react_client_test.dart b/test/react_client_test.dart
@@ -146,8 +146,7 @@ main() {
         final jsProps = unconvertJsProps(component);
         for (final key in knownEventKeys) {
           expect(jsProps[key], isNotNull, reason: 'JS event handler prop should not be null');
-          expect(
-              jsProps[key], anyOf(same(originalHandlers[key]), same(allowInterop(originalHandlers[key] as Function))),
+          expect(jsProps[key], anyOf(same(originalHandlers[key]), same(allowInterop(originalHandlers[key]!))),
               reason: 'JS event handler should be the original or original wrapped in allowInterop');
         }
       });
@@ -157,7 +156,7 @@ main() {
         final jsProps = unconvertJsProps(component);
         for (final key in knownEventKeys) {
           expect(jsProps[key], isNotNull, reason: 'JS event handler prop should not be null');
-          expect(jsProps[key], same(allowInterop(originalHandlers[key] as Function)),
+          expect(jsProps[key], same(allowInterop(originalHandlers[key]!)),
               reason: 'JS event handler prop was unexpectedly modified');
         }
       });
diff --git a/test/react_dom_test.dart b/test/react_dom_test.dart
@@ -0,0 +1,192 @@
+// ignore_for_file: deprecated_member_use_from_same_package
+@TestOn('browser')
+import 'dart:html';
+
+import 'package:react/react.dart' as react;
+import 'package:react/react_client/react_interop.dart';
+import 'package:react/react_dom.dart' as react_dom;
+import 'package:react/react_test_utils.dart';
+import 'package:test/test.dart';
+
+void main() {
+  // These tests redundantly test some React behavior, but mostly serve to validate that
+  // all supported inputs/outputs are handled properly by the Dart bindings and their typings.
+  group('react_dom entrypoint APIs:', () {
+    group('render', () {
+      group(
+          'accepts and renders content of different supported types,'
+          ' and returns a representation of what was mounted', () {
+        test('ReactElement for a DOM component', () {
+          final mountNode = DivElement();
+          final result = react_dom.render(react.button({}, 'test button'), mountNode);
+          expect(mountNode.children, [isA<ButtonElement>()]);
+          expect(mountNode.children.single.innerText, 'test button');
+          expect(result, isA<ButtonElement>());
+        });
+
+        test('ReactElement for a class component', () {
+          final mountNode = DivElement();
+          final result = react_dom.render(classComponent({}), mountNode);
+          expect(mountNode.innerText, 'class component content');
+          expect(result, isA<ReactComponent>().having((c) => c.dartComponent, 'dartComponent', isA<_ClassComponent>()));
+        });
+
+        test('ReactElement for a function component', () {
+          final mountNode = DivElement();
+          final result = react_dom.render(functionComponent({}), mountNode);
+          expect(mountNode.innerText, 'function component content');
+          expect(result, isNull);
+        });
+
+        group('other "ReactNode" types:', () {
+          test('string', () {
+            final mountNode = DivElement();
+            final result = react_dom.render('test string', mountNode);
+            expect(mountNode.innerText, 'test string');
+            expect(result, isA<Node>());
+          });
+
+          test('lists and nested lists', () {
+            final mountNode = DivElement();
+            final result = react_dom.render([
+              'test string',
+              ['test string 2', react.span({}, 'test span')]
+            ], mountNode);
+            expect(mountNode.innerText, 'test string' 'test string 2' 'test span');
+            expect(result, isA<Node>());
+          });
+
+          test('number', () {
+            final mountNode = DivElement();
+            final result = react_dom.render(123, mountNode);
+            expect(mountNode.innerText, '123');
+            expect(result, isA<Node>());
+          });
+
+          test('false', () {
+            final mountNode = DivElement();
+            react_dom.render(react.span({}, 'test content that will be cleared'), mountNode);
+            expect(mountNode.innerText, 'test content that will be cleared');
+            final result = react_dom.render(false, mountNode);
+            expect(mountNode.innerText, isEmpty);
+            expect(result, isNull);
+          });
+
+          test('null', () {
+            final mountNode = DivElement();
+            react_dom.render(react.span({}, 'test content that will be cleared'), mountNode);
+            expect(mountNode.innerText, 'test content that will be cleared');
+            final result = react_dom.render(null, mountNode);
+            expect(mountNode.innerText, isEmpty);
+            expect(result, isNull);
+          });
+        });
+      });
+    });
+
+    group('unmountComponentAtNode', () {
+      test('unmounts a React tree at a node, and returns true to indicate it has unmounted', () {
+        final mountNode = DivElement();
+        react_dom.render(react.span({}), mountNode);
+        final result = react_dom.unmountComponentAtNode(mountNode);
+        expect(result, isTrue);
+      });
+
+      test('returns false when a React tree has already been unmounted', () {
+        final mountNode = DivElement();
+        react_dom.render(react.span({}), mountNode);
+        final result = react_dom.unmountComponentAtNode(mountNode);
+        expect(result, isTrue, reason: 'test setup check');
+        final secondUnmountResult = react_dom.unmountComponentAtNode(mountNode);
+        expect(secondUnmountResult, isFalse);
+      });
+
+      test('returns false when no React tree has been mounted within a node', () {
+        final result = react_dom.unmountComponentAtNode(DivElement());
+        expect(result, isFalse);
+      });
+    });
+
+    group('findDOMNode', () {
+      group('returns the mounted element when provided', () {
+        test('a Dart class component instance', () {
+          final ref = createRef<_ClassComponent>();
+          renderIntoDocument(classComponent({'ref': ref}));
+          final dartComponentInstance = ref.current;
+          expect(dartComponentInstance, isNotNull, reason: 'test setup check');
+          dartComponentInstance!;
+          expect(react_dom.findDOMNode(dartComponentInstance), isA<AnchorElement>());
+        });
+
+        test('a JS class component instance', () {
+          final ref = createRef<_ClassComponent>();
+          renderIntoDocument(classComponent({'ref': ref}));
+          final jsComponentInstance = ref.current!.jsThis;
+          expect(jsComponentInstance, isNotNull, reason: 'test setup check');
+          expect(react_dom.findDOMNode(jsComponentInstance), isA<AnchorElement>());
+        });
+
+        test('an element representing a mounted component', () {
+          final ref = createRef<SpanElement>();
+          renderIntoDocument(react.span({'ref': ref}));
+          final element = ref.current;
+          expect(element, isNotNull, reason: 'test setup check');
+          element!;
+          expect(react_dom.findDOMNode(element), same(element));
+        });
+      });
+
+      test('returns null when a component does not render any content', () {
+        final ref = createRef<_ClassComponentThatRendersNothing>();
+        renderIntoDocument(classComponentThatRendersNothing({'ref': ref}));
+        final dartComponentInstance = ref.current;
+        expect(dartComponentInstance, isNotNull, reason: 'test setup check');
+        dartComponentInstance!;
+        expect(react_dom.findDOMNode(dartComponentInstance), isNull);
+      });
+
+      test('passes through a non-React-mounted element', () {
+        final element = DivElement();
+        expect(react_dom.findDOMNode(element), same(element));
+      });
+
+      test('passes through null', () {
+        expect(react_dom.findDOMNode(null), isNull);
+      });
+
+      group('throws when passed other objects that don\'t represent mounted React components', () {
+        test('arbitrary Dart objects', () {
+          expect(() => react_dom.findDOMNode(Object()),
+              throwsA(isA<Object>().havingToStringValue(contains('Argument appears to not be a ReactComponent'))));
+        });
+
+        test('ReactElement', () {
+          expect(() => react_dom.findDOMNode(react.span({})),
+              throwsA(isA<Object>().havingToStringValue(contains('Argument appears to not be a ReactComponent'))));
+        });
+      });
+    });
+  });
+}
+
+final classComponent = react.registerComponent2(() => _ClassComponent());
+
+class _ClassComponent extends react.Component2 {
+  @override
+  Object? render() => react.a({}, 'class component content');
+}
+
+final classComponentThatRendersNothing = react.registerComponent2(() => _ClassComponentThatRendersNothing());
+
+class _ClassComponentThatRendersNothing extends react.Component2 {
+  @override
+  Object? render() => null;
+}
+
+final functionComponent = react.registerFunctionComponent((props) {
+  return react.pre({}, 'function component content');
+});
+
+extension<T> on TypeMatcher<T> {
+  TypeMatcher<T> havingToStringValue(dynamic matcher) => having((o) => o.toString(), 'toString() value', matcher);
+}
diff --git a/test/react_dom_test.html b/test/react_dom_test.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<head lang="en">
+    <meta charset="UTF-8">
+    <title></title>
+    <script src="packages/react/react_with_addons.js"></script>
+    <script src="packages/react/react_dom.js"></script>
+    <link rel="x-dart-test" href="react_dom_test.dart">
+    <script src="packages/test/dart.js"></script>
+</head>
+<body>
+</body>
+</html>
diff --git a/test/react_test_utils_test.dart b/test/react_test_utils_test.dart
@@ -66,7 +66,7 @@ void testUtils({
 
     setUp(() {
       component = renderIntoDocument(eventComponent({})) as Object;
-      domNode = findDomNode(component);
+      domNode = findDomNode(component)!;
       expect(domNode.text, equals(''));
     });
 
@@ -350,12 +350,12 @@ void testUtils({
 
     expect(divElements.length, equals(3));
     // First div should be the parent div created by renderIntoDocument()
-    expect(findDomNode(divElements[0]).text, equals('A headerFirst divSecond div'));
-    expect(findDomNode(divElements[1]).text, equals('First div'));
-    expect(findDomNode(divElements[2]).text, equals('Second div'));
+    expect(findDomNode(divElements[0])!.text, equals('A headerFirst divSecond div'));
+    expect(findDomNode(divElements[1])!.text, equals('First div'));
+    expect(findDomNode(divElements[2])!.text, equals('Second div'));
     expect(h1Elements.length, equals(1));
-    expect(findDomNode(h1Elements[0]).text, equals('A header'));
+    expect(findDomNode(h1Elements[0])!.text, equals('A header'));
     expect(spanElements.length, equals(1));
-    expect(findDomNode(spanElements[0]).text, equals(''));
+    expect(findDomNode(spanElements[0])!.text, equals(''));
   });
 }
diff --git a/test/util.dart b/test/util.dart
@@ -33,7 +33,7 @@ bool isDartComponent2(ReactElement element) =>
 bool isDartComponent(ReactElement element) => ReactDartComponentVersion.fromType(element.type) != null;
 
 T getDartComponent<T extends react.Component>(dynamic dartComponent) {
-  return (dartComponent as ReactComponent).dartComponent as T;
+  return (dartComponent as ReactComponent).dartComponent! as T;
 }
 
 Map getDartComponentProps(dynamic dartComponent) {
@@ -49,7 +49,7 @@ ReactComponent render(ReactElement reactElement) {
 }
 
 // Same as the public API but with tightened types to help fix implicit casts
-Element findDomNode(dynamic component) => react_dom.findDOMNode(component) as Element;
+Element? findDomNode(dynamic component) => react_dom.findDOMNode(component) as Element?;
 
 /// Returns a new [Map.unmodifiable] with all argument maps merged in.
 Map unmodifiableMap([Map? map1, Map? map2, Map? map3, Map? map4]) {

Original file line number	Diff line number	Diff line change
`@@ -274,8 +274,8 @@ ReactComponentFactoryProxy memo2(ReactComponentFactoryProxy factory,`
`274`	`274`	`}`
`275`	`275`
`276`	`276`	`abstract class ReactDom {`
`277`		`- static Element findDOMNode(object) => ReactDOM.findDOMNode(object);`
`278`		`- static ReactComponent render(ReactElement component, Element element) => ReactDOM.render(component, element);`
	`277`	`+ static Element? findDOMNode(dynamic object) => ReactDOM.findDOMNode(object);`
	`278`	`+ static dynamic render(dynamic component, Element element) => ReactDOM.render(component, element);`
`279`	`279`	`static bool unmountComponentAtNode(Element element) => ReactDOM.unmountComponentAtNode(element);`
`280`	`280`
`281`	`281`	`/// Returns a a portal that renders [children] into a [container].`
Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ bool isDartComponent2(ReactElement element) =>`
`33`	`33`	`bool isDartComponent(ReactElement element) => ReactDartComponentVersion.fromType(element.type) != null;`
`34`	`34`
`35`	`35`	`T getDartComponent<T extends react.Component>(dynamic dartComponent) {`
`36`		`- return (dartComponent as ReactComponent).dartComponent as T;`
	`36`	`+ return (dartComponent as ReactComponent).dartComponent! as T;`
`37`	`37`	`}`
`38`	`38`
`39`	`39`	`Map getDartComponentProps(dynamic dartComponent) {`
`@@ -49,7 +49,7 @@ ReactComponent render(ReactElement reactElement) {`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`// Same as the public API but with tightened types to help fix implicit casts`
`52`		`-Element findDomNode(dynamic component) => react_dom.findDOMNode(component) as Element;`
	`52`	`+Element? findDomNode(dynamic component) => react_dom.findDOMNode(component) as Element?;`
`53`	`53`
`54`	`54`	`/// Returns a new [Map.unmodifiable] with all argument maps merged in.`
`55`	`55`	`Map unmodifiableMap([Map? map1, Map? map2, Map? map3, Map? map4]) {`