CPLAT-9691 Improve HOC Errors (#469)

* Add utility for validating component version

* Rework wording

* Add tests

* Clean up a little

* Add doc comments

* Fix failing tests

* Clean up a little

* Remove bad merge remainder

lib/src/component/ref_util.dart

@@ -53,6 +53,8 @@ Ref<T> createRef<T>() {
 
 /// Automatically passes a [Ref] through a component to one of its children.
 ///
+/// > __NOTE:__ This should only be used to wrap components that extend from `Component2`.
+///
 /// __Example__:
 ///
 ///     UiFactory<DomProps> DivForwarded = forwardRef<DomProps>((props, ref) {
@@ -95,6 +97,8 @@ UiFactory<TProps> Function(UiFactory<TProps>) forwardRef<TProps extends UiProps>
     Function(TProps props, Ref ref) wrapperFunction) {
 
   UiFactory<TProps> wrapWithForwardRef(UiFactory<TProps> factory) {
+    enforceMinimumComponentVersionFor(factory().componentFactory);
+
     Object wrapProps(Map props, Ref ref) {
       return wrapperFunction(factory(props), ref);
     }

lib/src/component_declaration/component_type_checking.dart

@@ -21,6 +21,7 @@ import 'package:meta/meta.dart';
 import 'package:over_react/src/component_declaration/component_base.dart' show UiFactory;
 import 'package:over_react/src/component_declaration/annotations.dart' as annotations show Component2;
 import 'package:over_react/src/util/react_wrappers.dart';
+import 'package:over_react/src/util/string_util.dart';
 import 'package:react/react_client.dart';
 import 'package:react/react_client/react_interop.dart';
 
@@ -243,7 +244,6 @@ bool isComponentOfType(ReactElement instance, dynamic typeAlias, {
     return false;
   }
 
-
   var instanceTypeMeta = getComponentTypeMeta(instanceType);
 
   // Type-check instance wrappers.
@@ -281,3 +281,18 @@ bool isComponentOfType(ReactElement instance, dynamic typeAlias, {
 bool isValidElementOfType(dynamic instance, dynamic typeAlias) {
   return isValidElement(instance) && isComponentOfType(instance, typeAlias);
 }
+
+/// Validates that a [ReactComponentFactoryProxy]'s component is not `Component`
+/// or `UiComponent`.
+void enforceMinimumComponentVersionFor(ReactComponentFactoryProxy component) {
+  if (component.type is String) return;
+
+  // ignore: invalid_use_of_protected_member
+  if (component.type.dartComponentVersion == '1') {
+    throw ArgumentError(unindent('''
+        The UiFactory provided should not be for a UiComponent or Component.
+        
+        Instead, use a different factory (such as UiComponent2 or Component2).
+        '''));
+  }
+}

lib/src/over_react_redux/over_react_flux.dart

@@ -290,9 +290,11 @@ bool _shallowMapEquality(Map a, Map b) => const MapEquality().equals(a, b);
 ///
 /// This is primarily for use while transitioning _to_ `connect` and OverReact Redux.
 ///
-/// __NOTE:__ Unlike `connect`, there is no `areStatesEqual` parameter due to the state
-/// update process being impure. It is impure because it involves modification of
-/// the store itself, as opposed to creating a new state object with each change.
+/// > __NOTE:__ This should only be used to wrap components that extend from [Component2].
+/// >
+/// > Additionally, unlike `connect`, there is no `areStatesEqual` parameter due to the state
+/// > update process being impure. It is impure because it involves modification of
+/// > the store itself, as opposed to creating a new state object with each change.
 ///
 /// __Example:__
 /// ```dart

lib/src/over_react_redux/over_react_redux.dart

@@ -45,6 +45,8 @@ typedef dynamic Dispatcher(dynamic action);
 
 /// A wrapper around the JS react-redux `connect` function that supports OverReact components.
 ///
+/// > __NOTE:__ This should only be used to wrap components that extend from [Component2].
+///
 /// __Example:__
 /// ```dart
 ///     UiFactory<CounterProps> ConnectedCounter = connect<CounterState, CounterProps>(
@@ -149,6 +151,10 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend
   areMergedPropsEqual ??= _shallowMapEquality;
 
   UiFactory<TProps> wrapWithConnect(UiFactory<TProps> factory) {
+    final dartComponentFactory = factory().componentFactory;
+    final dartComponentClass = dartComponentFactory.type;
+    enforceMinimumComponentVersionFor(dartComponentFactory);
+
     JsMap jsMapFromProps(Map props) => jsBackingMapOrJsCopy(props is UiProps ? props.props : props);
 
     TProps jsPropsToTProps(JsMap jsProps) => factory(JsBackedMap.backedBy(jsProps));
@@ -213,9 +219,6 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend
     bool handleAreMergedPropsEqual(JsMap jsNext, JsMap jsPrev) =>
         areMergedPropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev));
 
-    final dartComponentFactory = factory().componentFactory;
-    final dartComponentClass = dartComponentFactory.type;
-
     final hoc = mockableJsConnect(
       mapStateToProps != null ? allowInteropWithArgCount(handleMapStateToProps, 1) : mapStateToPropsWithOwnProps != null ? allowInteropWithArgCount(handleMapStateToPropsWithOwnProps, 2) : null,
       mapDispatchToProps != null ? allowInteropWithArgCount(handleMapDispatchToProps, 1) : mapDispatchToPropsWithOwnProps != null ? allowInteropWithArgCount(handleMapDispatchToPropsWithOwnProps, 2) : null,

test/over_react/component/fixtures/basic_ui_component.dart

@@ -0,0 +1,29 @@
+// Copyright 2019 Workiva Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import 'package:over_react/over_react.dart';
+
+part 'basic_ui_component.over_react.g.dart';
+// ignore_for_file: deprecated_member_use_from_same_package
+@Factory()
+UiFactory<BasicUiComponentProps> BasicUiComponent = _$BasicUiComponent;
+
+@Props()
+class _$BasicUiComponentProps extends UiProps {}
+
+@Component()
+class BasicUiComponentComponent extends UiComponent<BasicUiComponentProps> {
+  @override
+  render() => 'child component';
+}

test/over_react/component/forward_ref_test.dart

@@ -22,11 +22,20 @@ import 'package:over_react/src/component/dom_components.dart';
 
 import '../../test_util/test_util.dart';
 import '../component/fixtures/basic_child_component.dart';
+import 'fixtures/basic_ui_component.dart';
 
 part 'forward_ref_test.over_react.g.dart';
 
 main() {
   group('forward ref -', () {
+    test('errors when wrapping a UiComponent', () {
+      expect(() => forwardRef<BasicUiComponentProps>((props, ref) {
+        return (BasicUiComponent()
+          ..ref = ref
+        )();
+      })(BasicUiComponent), throwsArgumentError);
+    });
+
     group('on a component with a dom component child', () {
       forwardRefTest(Dom.span, verifyRefValue: (ref) {
         expect(ref, TypeMatcher<SpanElement>());

test/over_react/component_declaration/component_type_checking_test.dart

@@ -371,15 +371,25 @@ testComponentTypeChecking({
         });
 
         group('a higher-order component created by', () {
-          test('forwardRef', () {
-            final hocFactory = forwardRef((props, ref) => null)(TestA);
-            expect(isComponentOfType(hocFactory()(), TestA), isTrue);
-          });
+          if (TestA().componentFactory.type.dartComponentVersion == '1') {
+            test('forwardRef', () {
+              expect(() => forwardRef((props, ref) => null)(TestA), throwsArgumentError);
+            });
 
-          test('connect', () {
-            final hocFactory = connect(mapStateToProps: (state) => {})(TestA);
-            expect(isComponentOfType(hocFactory()(), TestA), isTrue);
-          });
+            test('connect', () {
+              expect(() => connect(mapStateToProps: (state) => {})(TestA), throwsArgumentError);
+            });
+          } else {
+            test('forwardRef', () {
+              final hocFactory = forwardRef((props, ref) => null)(TestA);
+              expect(isComponentOfType(hocFactory()(), TestA), isTrue);
+            });
+
+            test('connect', () {
+              final hocFactory = connect(mapStateToProps: (state) => {})(TestA);
+              expect(isComponentOfType(hocFactory()(), TestA), isTrue);
+            });
+          }
         });
       });
     });

test/over_react_redux/connect_flux_test.dart

@@ -21,6 +21,7 @@ import '../test_util/test_util.dart';
 import 'fixtures/connect_flux_counter.dart';
 import 'fixtures/connect_flux_store.dart';
 import 'fixtures/counter.dart';
+import 'fixtures/non_component_two_counter.dart';
 import 'fixtures/redux_actions.dart';
 import 'fixtures/store.dart' as redux_store;
 
@@ -64,6 +65,10 @@ main() {
     });
 
     group('behaves like redux with', () {
+      test('errors when wrapping a UiComponent', (){
+        expect(() => connectFlux<FluxStore, FluxActions, NonComponentTwoCounterProps>()(NonComponentTwoCounter), throwsArgumentError);
+      });
+
       group('Provider Usage', () {
         test('throws without a provider', () {
           ConnectedCounter =

test/over_react_redux/connect_test.dart

@@ -19,9 +19,10 @@ import 'package:over_react/over_react_redux.dart';
 import 'package:test/test.dart';
 
 import '../test_util/test_util.dart';
-import './fixtures/counter.dart';
-import './fixtures/redux_actions.dart';
-import './fixtures/store.dart';
+import 'fixtures/counter.dart';
+import 'fixtures/non_component_two_counter.dart';
+import 'fixtures/redux_actions.dart';
+import 'fixtures/store.dart';
 
 // ignore_for_file: avoid_types_on_closure_parameters
 
@@ -63,6 +64,10 @@ main() {
       await Future(() {});
     });
 
+    test('throws when mounting a UiComponent', () {
+      expect(() => connect<CounterState, NonComponentTwoCounterProps>()(NonComponentTwoCounter), throwsArgumentError);
+    });
+
     group('Provider Usage', () {
       test('throws without a provider', () {
         ConnectedCounter = connect<CounterState, CounterProps>()(Counter);

test/over_react_redux/fixtures/non_component_two_counter.dart

@@ -0,0 +1,18 @@
+import 'package:over_react/over_react.dart';
+import 'package:over_react/over_react_redux.dart';
+
+part 'non_component_two_counter.over_react.g.dart';
+// ignore_for_file: deprecated_member_use_from_same_package
+@Factory()
+UiFactory<NonComponentTwoCounterProps> NonComponentTwoCounter = _$NonComponentTwoCounter;
+
+@Props()
+class _$NonComponentTwoCounterProps extends UiProps with ConnectPropsMixin {}
+
+@Component()
+class NonComponentTwoCounterComponent extends UiComponent<NonComponentTwoCounterProps> {
+  @override
+  render() {
+    return Dom.div()();
+  }
+}