Skip to content

Commit

Permalink
Merge pull request #382 from Workiva/fix-react-dom-typings
Browse files Browse the repository at this point in the history
FED-1910 Fix react_dom API typings, add tests
  • Loading branch information
rmconsole5-wk authored Nov 17, 2023
2 parents c0ccd1b + 9f6993e commit c59ba12
Show file tree
Hide file tree
Showing 9 changed files with 241 additions and 18 deletions.
19 changes: 19 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/react_client/react_interop.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Expand Down
4 changes: 2 additions & 2 deletions lib/src/react_client/dart2_interop_workaround_bindings.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
6 changes: 3 additions & 3 deletions test/lifecycle_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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});

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
5 changes: 2 additions & 3 deletions test/react_client_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
});
Expand All @@ -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');
}
});
Expand Down
192 changes: 192 additions & 0 deletions test/react_dom_test.dart
Original file line number Diff line number Diff line change
@@ -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);
}
13 changes: 13 additions & 0 deletions test/react_dom_test.html
Original file line number Diff line number Diff line change
@@ -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>
12 changes: 6 additions & 6 deletions test/react_test_utils_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ void testUtils({

setUp(() {
component = renderIntoDocument(eventComponent({})) as Object;
domNode = findDomNode(component);
domNode = findDomNode(component)!;
expect(domNode.text, equals(''));
});

Expand Down Expand Up @@ -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(''));
});
}
4 changes: 2 additions & 2 deletions test/util.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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]) {
Expand Down

0 comments on commit c59ba12

Please sign in to comment.