diff --git a/CHANGELOG.md b/CHANGELOG.md index a6d0fd41..39fa8c94 100644 --- 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 index 278430b0..4c2b46a7 100644 --- 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 index 69f8e25b..815b7bf4 100644 --- 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 index b3dd3c58..b7e17565 100644 --- 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(renderedInstance); - final renderedNode = findDomNode(renderedInstance); + final renderedNode = findDomNode(renderedInstance)!; LifecycleTestHelper.staticLifecycleCalls.clear(); component.setState({'shouldThrow': true}); @@ -1518,7 +1518,7 @@ void sharedLifecycleTests({ 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(renderedInstance); react_test_utils.Simulate.click(renderedNode.children.first); @@ -1541,7 +1541,7 @@ void sharedLifecycleTests({ 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(renderedInstance); react_test_utils.Simulate.click(renderedNode.children.first); diff --git a/test/react_client_test.dart b/test/react_client_test.dart index 97aed13a..373e64b2 100644 --- 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 new file mode 100644 index 00000000..f933d847 --- /dev/null +++ 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()]); + expect(mountNode.children.single.innerText, 'test button'); + expect(result, isA()); + }); + + 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().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()); + }); + + 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()); + }); + + test('number', () { + final mountNode = DivElement(); + final result = react_dom.render(123, mountNode); + expect(mountNode.innerText, '123'); + expect(result, isA()); + }); + + 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()); + }); + + 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()); + }); + + test('an element representing a mounted component', () { + final ref = createRef(); + 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().havingToStringValue(contains('Argument appears to not be a ReactComponent')))); + }); + + test('ReactElement', () { + expect(() => react_dom.findDOMNode(react.span({})), + throwsA(isA().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 on TypeMatcher { + TypeMatcher havingToStringValue(dynamic matcher) => having((o) => o.toString(), 'toString() value', matcher); +} diff --git a/test/react_dom_test.html b/test/react_dom_test.html new file mode 100644 index 00000000..c218f566 --- /dev/null +++ b/test/react_dom_test.html @@ -0,0 +1,13 @@ + + + + + + + + + + + + + diff --git a/test/react_test_utils_test.dart b/test/react_test_utils_test.dart index 1f4ef2b3..8dc96217 100644 --- 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 index 042db617..a5c7a236 100644 --- 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(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]) {