Skip to content

FED-1910 Fix react_dom API typings, add tests #382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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