Skip to content
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

[OLD] FED 2299 Utilize new ReactNode typedef #893

Closed
wants to merge 12 commits into from
3 changes: 2 additions & 1 deletion lib/over_react.dart
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ export 'package:react/react_client.dart'
setClientConfiguration,
chainRefs,
ReactElement,
ReactComponentFactoryProxy;
ReactComponentFactoryProxy,
ReactNode;
export 'package:react/react_client/react_interop.dart' show ReactErrorInfo, Ref;
export 'package:react/hooks.dart' show StateHook, ReducerHook;

Expand Down
2 changes: 1 addition & 1 deletion lib/react_dom.dart
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import 'package:react/react_dom.dart' as react_dom show render, unmountComponent
/// Use [unmountComponentAtNode] to unmount the instance.
///
/// > Proxies [react_dom.render].
dynamic render(/*ReactNode*/ dynamic element, Element mountNode) {
dynamic render(ReactNode element, Element mountNode) {
return react_dom.render(element, mountNode);
}

Expand Down
4 changes: 2 additions & 2 deletions lib/src/component/_deprecated/error_boundary_mixins.dart
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ abstract class _$ErrorBoundaryPropsMixin implements UiProps {
/// component tree that crashed.
///
/// > Related: [onComponentIsUnrecoverable], [onComponentDidCatch]
ReactElement Function(/*Error||Exception*/dynamic error, ReactErrorInfo? info)? fallbackUIRenderer;
ReactNode Function(/*Error||Exception*/dynamic error, ReactErrorInfo? info)? fallbackUIRenderer;

/// The amount of time that is "acceptable" between consecutive identical errors thrown from a component
/// within the tree wrapped by this [ErrorBoundary].
Expand Down Expand Up @@ -361,7 +361,7 @@ mixin ErrorBoundaryMixin<T extends ErrorBoundaryPropsMixin, S extends ErrorBound
}

// [2.2]
ReactElement? _renderStringDomAfterUnrecoverableErrors(_, __) {
ReactNode _renderStringDomAfterUnrecoverableErrors(_, __) {
return (Dom.div()
..key = 'ohnoes'
..addTestId('ErrorBoundary.unrecoverableErrorInnerHtmlContainerNode')
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/src/component/error_boundary.dart
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ mixin ErrorBoundaryProps on UiProps {
/// component tree that crashed.
///
/// > Related: [onComponentIsUnrecoverable], [onComponentDidCatch]
ReactElement? Function(/*Error||Exception*/dynamic error, ReactErrorInfo? info)? fallbackUIRenderer;
ReactNode Function(/*Error||Exception*/dynamic error, ReactErrorInfo? info)? fallbackUIRenderer;

/// The amount of time that is "acceptable" between consecutive identical errors thrown from a component
/// within the tree wrapped by this [ErrorBoundary].
Expand Down
6 changes: 3 additions & 3 deletions lib/src/component/error_boundary.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/src/component/error_boundary_recoverable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ class RecoverableErrorBoundaryComponent<T extends RecoverableErrorBoundaryProps,
}

// [2.2]
ReactElement? _renderStringDomAfterUnrecoverableErrors(_, __) {
ReactNode _renderStringDomAfterUnrecoverableErrors(_, __) {
return (Dom.div()
..key = 'ohnoes'
..addTestId('ErrorBoundary.unrecoverableErrorInnerHtmlContainerNode')
Expand Down
8 changes: 4 additions & 4 deletions lib/src/component/prop_mixins.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
/// Various prop related mixins to be used with `UiComponent` descendants.
library over_react.prop_mixins;

import 'package:over_react/over_react.dart' show AriaPropsMapView, AriaPropsMixin, DomProps, PropsMeta;
import 'package:over_react/over_react.dart' show AriaPropsMapView, AriaPropsMixin, DomProps, PropsMeta, ReactNode;
// Must import these consts because they are used in the transformed code.
// ignore: deprecated_member_use, unused_shown_name
import 'package:over_react/over_react.dart' show PropDescriptor, ConsumedProps, PropsMeta;
Expand Down Expand Up @@ -46,10 +46,10 @@ abstract class _$ReactPropsMixin {

// This private field is namespaced to avoid colliding with other classes.
@Accessor(key: 'children')
dynamic _raw$ReactProps$children;
ReactNode _raw$ReactProps$children;

/// The children that were passed in to this component when it was built.
List<dynamic>? get children {
List<ReactNode>? get children {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is a breaking change, since consumers will no longer be able to dynamically invoke methods on children (which they probably shouldn't be doing, and instead using proper type checks).

And I think it might mess up implicit casting in some cases too, not 100% sure.

I'd just be surprised if there aren't some usages out there that would be broken.

So, I'd say either we revert this change, or if we really wanted to, perhaps we could do some consumer static analysis testing to see if it affects anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check did not do well. I'll revert this.

Copy link
Contributor Author

@aaronlademann-wf aaronlademann-wf Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final value = _raw$ReactProps$children;

// Most common case; Dart components should all have List children
Expand All @@ -68,7 +68,7 @@ abstract class _$ReactPropsMixin {
return [value];
}

set children(List<dynamic>? value) => _raw$ReactProps$children = value;
set children(List<ReactNode>? value) => _raw$ReactProps$children = value;

/// A String that differentiates a component from its siblings.
///
Expand Down
10 changes: 5 additions & 5 deletions lib/src/component/prop_mixins.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions lib/src/component/ref_util.dart
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ Ref<T?> createRef<T>() => react_interop.createRef();
/// _$Foo2Config, // ignore: undefined_identifier
/// );
UiFactory<TProps> uiForwardRef<TProps extends bh.UiProps>(
dynamic Function(TProps props, dynamic ref) functionComponent, dynamic _config) {
/*ReactNode*/ dynamic Function(TProps props, dynamic ref) functionComponent, dynamic _config) {
ArgumentError.checkNotNull(_config, '_config');

if (_config is! UiFactoryConfig<TProps>) {
Expand All @@ -231,7 +231,7 @@ UiFactory<TProps> uiForwardRef<TProps extends bh.UiProps>(
// this will be an empty string.
final displayName = config.displayName ?? getFunctionName(functionComponent);

dynamic _uiFunctionWrapper(JsBackedMap props, dynamic ref) {
/*ReactNode*/ dynamic _uiFunctionWrapper(JsBackedMap props, dynamic ref) {
return functionComponent(propsFactory!.jsMap(props), ref);
}

Expand Down
4 changes: 2 additions & 2 deletions lib/src/component/suspense_component.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ mixin SuspensePropsMixin on UiProps {
/// The actual UI you intend to render. If children suspends while rendering, the Suspense boundary will
/// switch to rendering fallback.
@override
/*ReactNode*/ List<dynamic>? get children;
List<ReactNode>? get children;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, the changes to props in this component are technically breaking changes, but I think we could get away with these in particular since, by comparison, there should be almost no components that read the children/fallback props from this props mixin.


/// An alternate UI to render in place of the actual UI if it has not finished loading. Any valid React node is
/// accepted, though in practice, a fallback is a lightweight placeholder view, such as a loading spinner or skeleton.
/// Suspense will automatically switch to fallback when children suspends, and back to children when the data is ready.
/// If fallback suspends while rendering, it will activate the closest parent Suspense boundary.
/*ReactNode*/ dynamic fallback;
ReactNode fallback;
}
6 changes: 3 additions & 3 deletions lib/src/component/suspense_component.over_react.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions lib/src/component/with_transition.dart
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ class WithTransitionComponent extends UiStatefulComponent2<WithTransitionProps,
render() {
assert(_hasSingleValidChild(props));

// ok to ignore because of the above assert
// ignore: cast_nullable_to_non_nullable
final childElement = props.children!.single as ReactElement;
final childProps = domProps(getProps(childElement));
final phaseProps = props.childPropsByPhase![state.$transitionPhase] ?? const {};
Expand Down
4 changes: 2 additions & 2 deletions lib/src/component_declaration/component_base.dart
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ abstract class UiProps extends MapBase
}

/// Returns a new component with this builder's [props] and the specified [children].
ReactElement build([dynamic children]) {
ReactElement build([ReactNode children]) {
assert(_validateChildren(children));
_sharedAsserts();

Expand Down Expand Up @@ -643,7 +643,7 @@ abstract class UiProps extends MapBase

/// Validates that no [children] are instances of [UiProps], and prints a helpful message for a better debugging
/// experience.
bool _validateChildren(dynamic children) {
bool _validateChildren(ReactNode children) {
// Should not validate non-list iterables to avoid more than one iteration.
if (children != null && (children is! Iterable || children is List)) {
final childrenList = children is List ? children : [children];
Expand Down
2 changes: 1 addition & 1 deletion lib/src/component_declaration/function_component.dart
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export 'component_type_checking.dart'
/// Learn more: <https://reactjs.org/docs/components-and-props.html#function-and-class-components>.
// TODO: right now only top level factory declarations will generate props configs.
UiFactory<TProps> uiFunction<TProps extends UiProps>(
dynamic Function(TProps props) functionComponent,
/*ReactNode*/ dynamic Function(TProps props) functionComponent,
dynamic _config,
) {
ArgumentError.checkNotNull(_config, '_config');
Expand Down
2 changes: 1 addition & 1 deletion lib/src/over_react_redux/redux_multi_provider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class ReduxMultiProviderComponent

@override
render() {
dynamic content = props.children;
ReactNode content = props.children;
props.storesByContext.forEach((context, store) {
content = (ReduxProvider()
..store = store
Expand Down
2 changes: 1 addition & 1 deletion lib/src/util/react_util.dart
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ abstract class UiPropsMapView extends UiProps {
Map get componentDefaultProps => throw UnimplementedError('@PropsMixin instances do not implement defaultProps');

@override
ReactElement build([dynamic children]) =>
ReactElement build([ReactNode children]) =>
throw UnimplementedError('@PropsMixin instances do not implement build');

@override
Expand Down
10 changes: 5 additions & 5 deletions lib/src/util/react_wrappers.dart
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ Map getProps(dynamic/* ReactElement|ReactComponent */ instance, {bool traverseWr
///
/// This method simply wraps react.findDOMNode with strong typing for the return value
/// (and for the function itself, which is declared using `var` in react-dart).
Element? findDomNode(dynamic instance) => react_dom.findDOMNode(instance) as Element?;
Element? findDomNode(ReactNode instance) => react_dom.findDOMNode(instance) as Element?;
aaronlademann-wf marked this conversation as resolved.
Show resolved Hide resolved

/// Returns a portal that renders [children] into a [container].
///
Expand All @@ -204,7 +204,7 @@ Element? findDomNode(dynamic instance) => react_dom.findDOMNode(instance) as Ele
/// [children] can be any renderable React child, such as a [ReactElement], [String], or fragment.
///
/// See: <https://reactjs.org/docs/portals.html>
ReactPortal createPortal(dynamic children, Element container) => ReactDom.createPortal(children, container);
ReactPortal createPortal(ReactNode children, Element container) => ReactDom.createPortal(children, container);

/// Dart wrapper for React.isValidElement.
///
Expand All @@ -223,7 +223,7 @@ bool isDomElement(dynamic instance) {
/// Returns whether [instance] is a composite [ReactComponent].
///
/// __Not for external use.__
bool _isCompositeComponent(Object? instance) {
bool _isCompositeComponent(ReactNode instance) {
aaronlademann-wf marked this conversation as resolved.
Show resolved Hide resolved
return instance != null && getProperty(instance, 'isReactComponent') != null;
}

Expand All @@ -240,7 +240,7 @@ bool _isCompositeComponent(Object? instance) {
/// * Children are likewise copied and potentially overwritten with [newChildren] as expected.
/// * For JS components, a JS copy of [newProps] is returned, since React will merge the props without any special handling.
/// If these values might contain event handlers
dynamic preparePropsChangeset(ReactElement element, Map? newProps, [Iterable? newChildren]) {
dynamic preparePropsChangeset(ReactElement element, Map? newProps, [Iterable<ReactNode>? newChildren]) {
final type = element.type;
final dartComponentVersion = ReactDartComponentVersion.fromType(type); // ignore: invalid_use_of_protected_member

Expand Down Expand Up @@ -294,7 +294,7 @@ external ReactElement _cloneElement(element, [props, children]);
/// > Unlike React.addons.cloneWithProps, key and ref from the original element will be preserved.
/// > There is no special behavior for merging any props (unlike cloneWithProps).
/// > See the [v0.13 RC2 blog post](https://facebook.github.io/react/blog/2015/03/03/react-v0.13-rc2.html) for additional details.
ReactElement cloneElement(ReactElement element, [Map? props, Iterable? children]) {
ReactElement cloneElement(ReactElement element, [Map? props, Iterable<ReactNode>? children]) {
ArgumentError.checkNotNull(element, 'element');

var propsChangeset = preparePropsChangeset(element, props, children);
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ dependencies:
meta: ^1.6.0
package_config: ^2.1.0
path: ^1.5.1
react: ^7.0.0
react: ^7.1.0
redux: '>=5.0.0 <6.0.0'
source_span: ^1.4.1
transformer_utils: ^0.2.6
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ main() {
});

test('null: returns an empty list', () {
expect(getTypedView({childrenKey: null}).children, same(const []));
expect(getTypedView({childrenKey: null}).children, same(const <ReactNode>[]));
});

test('a single child: wraps in a list', () {
Expand Down